bitcoin-core bundles leveldb

OpenSubmitted by Danny Milosavljevic.
Details
4 participants
  • Carl Dong
  • Danny Milosavljevic
  • Maxime Devos
  • zimoun
Owner
unassigned
Severity
normal
D
D
Danny Milosavljevic wrote on 22 Jan 2019 13:31
(address . bug-guix@gnu.org)
20190122133124.7ca032d7@scratchpost.org
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAlxHDRwACgkQ5xo1VCww
uqU4qQf/TF0NXXkLjk7McWCSIzVN7oFXQkyj1k5m7TfdL3Ztw72dPM9X4F8/9nC5
zt/sJrD4Qi1M2DOSTRKb4Bmfbr9WdtEC6yRU576UK0pijlHgO8M0rdaXnGTO/NC6
nXLssXWJYhtmIzZUsYW7W7a4B72pRSEhRbfPDNbASVBGzdJAUKn8lgQoEym7HSSR
UT2qcO/G267+YSQY9ZowqNy1hskTZHeaY0H3d/w4mkDmRj0I/O/50JhYJxNO/Xjw
xeH2I/RqNmq+1i783m91bUA+eylwm0ldOwUAdL0XcThd/Zy/lJ7OVm14Gj7dZGJ9
hRSQNrUKcVGwqvRdUFG2lwesSIm0wg==
=0Km+
-----END PGP SIGNATURE-----


Z
Z
zimoun wrote on 3 Dec 2021 11:29
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
865ys6m168.fsf@gmail.com
Hi,

This old bug#34170 [1] provides only the ’Subject’ as elements.
The package bitcore-core still provides ’leveldb’:

Toggle snippet (21 lines)
$ tar xf $(guix build -S bitcoin-core)
$ ls -1 bitcoin-0.21.2/src/leveldb
AUTHORS
benchmarks
cmake
CMakeLists.txt
CONTRIBUTING.md
db
doc
helpers
include
issues
LICENSE
NEWS
port
README.md
table
TODO
util

and I am not sure to get what is the issue. Some explanations?



Cheers,
simon
Z
Z
zimoun wrote on 5 Jan 00:29 +0100
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)
86a6gbaxmm.fsf@gmail.com
Hi,

On Fri, 03 Dec 2021 at 11:29, zimoun <zimon.toutoune@gmail.com> wrote:

Toggle quote (27 lines)
> This old bug#34170 [1] provides only the ’Subject’ as elements.
> The package bitcore-core still provides ’leveldb’:
>
> $ tar xf $(guix build -S bitcoin-core)
> $ ls -1 bitcoin-0.21.2/src/leveldb
> AUTHORS
> benchmarks
> cmake
> CMakeLists.txt
> CONTRIBUTING.md
> db
> doc
> helpers
> include
> issues
> LICENSE
> NEWS
> port
> README.md
> table
> TODO
> util
>
> and I am not sure to get what is the issue. Some explanations?
>
> 1: <http://issues.guix.gnu.org/issue/34170>

I am closing. Feel free to reopen if I miss something.


Cheers,
simon
Closed
M
M
Maxime Devos wrote on 5 Jan 10:39 +0100
df96d7bdccb0a6a4ae1b7aa9da8c8d30f97360c3.camel@telenet.be
zimoun schreef op vr 03-12-2021 om 11:29 [+0100]:
Toggle quote (9 lines)
> Hi,
>
> This old bug#34170 [1] provides only the ’Subject’ as elements.
> The package bitcore-core still provides ’leveldb’:
>
> [...]
>
> and I am not sure to get what is the issue. Some explanations?

The issue is that bitcoin-core bundles leveldb (which you have shown
is still the case), even though leveldb is packaged in Guix. See, e.g.
‘(guix)Snippets versus Phases’ and the following from ‘(guix)Submitting
Patches’:

7. Make sure the package does not use bundled copies of software
already available as separate packages.

Sometimes, packages include copies of the source code of their
dependencies as a convenience for users. However, as a
distribution, we want to make sure that such packages end up using
the copy we already have in the distribution, if there is one.
This improves resource usage (the dependency is built and stored
only once), and allows the distribution to make transverse changes
such as applying security updates for a given software package in a
single place and have them affect the whole system—something that
bundled copies prevent.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYdVnRxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7rbGAQDs9M72x4vWmzGBMDCgAS3xvuGS
WLrER2FZy3j4vEi2HwEA+yb+GJB1iN2vBY0t9rMHnjerWYCEyXm5JP7bAjI3BgI=
=cyUF
-----END PGP SIGNATURE-----


Z
Z
zimoun wrote on 5 Jan 10:45 +0100
(name . Maxime Devos)(address . maximedevos@telenet.be)
86lezu8qj4.fsf@gmail.com
Hi Maxime,

On Wed, 05 Jan 2022 at 10:39, Maxime Devos <maximedevos@telenet.be> wrote:

Toggle quote (9 lines)
>> The package bitcore-core still provides ’leveldb’:
>>
>> [...]
>>
>> and I am not sure to get what is the issue. Some explanations?
>
> The issue is that bitcoin-core bundles leveldb (which you have shown
> is still the case), even though leveldb is packaged in Guix.

Thanks, I missed that ’leveldb’ is packaged in Guix. Indeed, it should
preferably be used.

So reopen. :-)


Cheers,
simon
C
C
Carl Dong wrote on 5 Jan 18:40 +0100
(name . zimoun)(address . zimon.toutoune@gmail.com)
26B84833-6F9F-4C5D-8448-715C67C16768@carldong.me
Simon, Maxime, Danny,

Thanks for CCing me on this message! The rationale for bundling leveldb in Bitcoin Core goes a bit beyond convenience, it is several things:

1. The original reason for sub-treeing is that Bitcoin Core used to maintain its own version of leveldb with its own fixes here: https://github.com/bitcoin-core/leveldb-subtree https://github.com/bitcoin-core/leveldb-subtree , since then most of these fixes have been upstreamed as of: https://github.com/bitcoin/bitcoin/pull/17398 https://github.com/bitcoin/bitcoin/pull/17398
2. We also used to support using an external leveldb, however, it seems that it was fragile to rely on external projects to maintain ABI compatibility, see the quoted IRC bug report here: https://github.com/bitcoin/bitcoin/pull/23282 https://github.com/bitcoin/bitcoin/pull/23282 . Reasonable minds may disagree on this point, especially coming from Guix where patching is convenient.

In addition to the above, Bitcoin Core experienced a hard fork in 2013 due to database incompatibilities, which has predisposed maintainers towards a more stringent approach with pinning dependencies and their configure/build-time flags. See: https://blog.bitmex.com/bitcoins-consensus-forks/#was-the-2013-incident-a-hardfork https://blog.bitmex.com/bitcoins-consensus-forks/#was-the-2013-incident-a-hardfork

Let me know if I can provide more context!

Cheers,
Carl Dong

Toggle quote (23 lines)
> On Jan 5, 2022, at 4:45 AM, zimoun <zimon.toutoune@gmail.com> wrote:
>
> Hi Maxime,
>
> On Wed, 05 Jan 2022 at 10:39, Maxime Devos <maximedevos@telenet.be> wrote:
>
>>> The package bitcore-core still provides ’leveldb’:
>>>
>>> [...]
>>>
>>> and I am not sure to get what is the issue. Some explanations?
>>
>> The issue is that bitcoin-core bundles leveldb (which you have shown
>> is still the case), even though leveldb is packaged in Guix.
>
> Thanks, I missed that ’leveldb’ is packaged in Guix. Indeed, it should
> preferably be used.
>
> So reopen. :-)
>
>
> Cheers,
> simon
Attachment: file
M
M
Maxime Devos wrote on 5 Jan 20:13 +0100
563546ec57ff4726bf4138c16be6304c869119ce.camel@telenet.be
Hi,

Carl Dong schreef op wo 05-01-2022 om 12:40 [-0500]:
Toggle quote (12 lines)
> Simon, Maxime, Danny,
>
> Thanks for CCing me on this message! The rationale for bundling
> leveldb in Bitcoin Core goes a bit beyond convenience, it is several
> things:
>
> 1. The original reason for sub-treeing is that Bitcoin Core used to
> maintain its own version of leveldb with its own fixes
> here: https://github.com/bitcoin-core/leveldb-subtree, since then
> most of these fixes have been upstreamed as
> of: https://github.com/bitcoin/bitcoin/pull/17398

Seems reasonable to me, but the bitcoin project is upstreaming the
changes it made and most are already upstream, so I would prefer
to use upstream's leveldb.

Toggle quote (7 lines)
> 2. We also used to support using an external leveldb, however, it
> seems that it was fragile to rely on external projects to maintain
> ABI compatibility, see the quoted IRC bug report
> here: https://github.com/bitcoin/bitcoin/pull/23282. Reasonable minds
> may disagree on this point, especially coming from Guix where
> patching is convenient.

The quoted ABI incompatibility

<Talkless> bitcoind fails to start with undefined symbol:
_ZTIN7leveldb6LoggerE in Debian Sid after leveldb upgraded from 1.22 to

doesn't apply in Guix, because guix uses RUNPATH and multiple library
versions can exist in the same store (in different directories in the
store).

Toggle quote (7 lines)
> In addition to the above, Bitcoin Core experienced a hard fork in
> 2013 due to database incompatibilities, which has predisposed
> maintainers towards a more stringent approach with pinning
> dependencies and their configure/build-time flags.
> See: https://blog.bitmex.com/bitcoins-consensus-forks/#was-the-2013-
> incident-a-hardfork

I doubt that Guix has sufficient Bitcoin Core users to cause
a hard fork, but yes, this is an understandable reason to bundle
things. But any such problem seems easy to resolve (at the guix side):
we could simply temporarily switch to an older version of leveldb.

If desired, it would also be possible to do something in-between
unbundling and using bitcoin's leveldb: define a 'leveldb/bitcoin'
variant of the 'leveldb' package (using package/inherit or (package
(inherit ...) ...)), add it as input to the 'bitcoin' package and tell
and/or patch bitcoin's buid scripts to use that leveldb.

As source code, use an appropriate commit from
to the definition of bitcoin-core to keep leveldb/bitcoin in-sync).

A benefit of this approach (if done properly, with (origin (inherit
...) ...) such that patches of 'leveldb' are inherited) above the
status quo, is that is that if for some reason 'leveldb' is patched in
Guix, then 'leveldb/bitcoin' receives the patch as well. Another
benefit is that the dependency 'googletest' and 'benchmark' of leveldb
would remain unbundled.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYdXt9RccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7hu6AQDW+ScMM2tEpvFe43jOShlsOqpP
dx6ANkesoO7UUd9PZwD/VNJ4E+LxegBqgypK8t/JFixNJ1YAfCgdb9wkC2XezgU=
=mfVJ
-----END PGP SIGNATURE-----


C
C
Carl Dong wrote on 7 Jan 01:17 +0100
(name . Maxime Devos)(address . maximedevos@telenet.be)
70A07344-0DA9-4A8E-B3B0-FAE1721FE2D2@carldong.me
Toggle quote (5 lines)
> If desired, it would also be possible to do something in-between
> unbundling and using bitcoin's leveldb: define a 'leveldb/bitcoin'
> variant of the 'leveldb' package (using package/inherit or (package
> (inherit ...) ...)), add it as input to the 'bitcoin' package and tell
> and/or patch bitcoin's buid scripts to use that leveldb.
Yes I think that would be a splendid idea! With regards to patching bitcoin’s builds scripts, Bitcoin Knots follows Bitcoin Core closely, but has a bunch of patches which allow for using system libs, so that might be good to reference: https://github.com/bitcoin/bitcoin/compare/master...bitcoinknots:21.x-syslibs https://github.com/bitcoin/bitcoin/compare/master...bitcoinknots:21.x-syslibs

Toggle quote (4 lines)
> As source code, use an appropriate commit from
> <https://github.com/bitcoin-core/leveldb-subtree> (and add a comment
> to the definition of bitcoin-core to keep leveldb/bitcoin in-sync).

FYI, according to https://github.com/bitcoin/bitcoin/pull/17398 https://github.com/bitcoin/bitcoin/pull/17398 , we are currently using the upstream LevelDB commit 0c40829872a9f00f38e11dc370ff8adb3e19f25b

Cheers,
Carl Dong


Toggle quote (70 lines)
> On Jan 5, 2022, at 2:13 PM, Maxime Devos <maximedevos@telenet.be> wrote:
>
> Hi,
>
> Carl Dong schreef op wo 05-01-2022 om 12:40 [-0500]:
>> Simon, Maxime, Danny,
>>
>> Thanks for CCing me on this message! The rationale for bundling
>> leveldb in Bitcoin Core goes a bit beyond convenience, it is several
>> things:
>>
>> 1. The original reason for sub-treeing is that Bitcoin Core used to
>> maintain its own version of leveldb with its own fixes
>> here: https://github.com/bitcoin-core/leveldb-subtree, since then
>> most of these fixes have been upstreamed as
>> of: https://github.com/bitcoin/bitcoin/pull/17398
>
> Seems reasonable to me, but the bitcoin project is upstreaming the
> changes it made and most are already upstream, so I would prefer
> to use upstream's leveldb.
>
>> 2. We also used to support using an external leveldb, however, it
>> seems that it was fragile to rely on external projects to maintain
>> ABI compatibility, see the quoted IRC bug report
>> here: https://github.com/bitcoin/bitcoin/pull/23282. Reasonable minds
>> may disagree on this point, especially coming from Guix where
>> patching is convenient.
>
> The quoted ABI incompatibility
>
> <Talkless> bitcoind fails to start with undefined symbol:
> _ZTIN7leveldb6LoggerE in Debian Sid after leveldb upgraded from 1.22 to
> 1.23: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=996486
>
> doesn't apply in Guix, because guix uses RUNPATH and multiple library
> versions can exist in the same store (in different directories in the
> store).
>
>> In addition to the above, Bitcoin Core experienced a hard fork in
>> 2013 due to database incompatibilities, which has predisposed
>> maintainers towards a more stringent approach with pinning
>> dependencies and their configure/build-time flags.
>> See: https://blog.bitmex.com/bitcoins-consensus-forks/#was-the-2013-
>> incident-a-hardfork
>
> I doubt that Guix has sufficient Bitcoin Core users to cause
> a hard fork, but yes, this is an understandable reason to bundle
> things. But any such problem seems easy to resolve (at the guix side):
> we could simply temporarily switch to an older version of leveldb.
>
> If desired, it would also be possible to do something in-between
> unbundling and using bitcoin's leveldb: define a 'leveldb/bitcoin'
> variant of the 'leveldb' package (using package/inherit or (package
> (inherit ...) ...)), add it as input to the 'bitcoin' package and tell
> and/or patch bitcoin's buid scripts to use that leveldb.
>
> As source code, use an appropriate commit from
> <https://github.com/bitcoin-core/leveldb-subtree> (and add a comment
> to the definition of bitcoin-core to keep leveldb/bitcoin in-sync).
>
> A benefit of this approach (if done properly, with (origin (inherit
> ...) ...) such that patches of 'leveldb' are inherited) above the
> status quo, is that is that if for some reason 'leveldb' is patched in
> Guix, then 'leveldb/bitcoin' receives the patch as well. Another
> benefit is that the dependency 'googletest' and 'benchmark' of leveldb
> would remain unbundled.
>
> Greetings,
> Maxime.
>
Attachment: file
Attachment: signature.asc
?