unofficial mirror of bug-guix@gnu.org 
 help / color / mirror / code / Atom feed
* bug#34170: bitcoin-core bundles leveldb
@ 2019-01-22 12:31 Danny Milosavljevic
  2021-12-03 10:29 ` zimoun
  0 siblings, 1 reply; 9+ messages in thread
From: Danny Milosavljevic @ 2019-01-22 12:31 UTC (permalink / raw)
  To: 34170

[-- Attachment #1: Type: text/plain, Size: 0 bytes --]



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#34170: bitcoin-core bundles leveldb
  2019-01-22 12:31 bug#34170: bitcoin-core bundles leveldb Danny Milosavljevic
@ 2021-12-03 10:29 ` zimoun
  2022-01-04 23:29   ` zimoun
  2022-01-05  9:39   ` Maxime Devos
  0 siblings, 2 replies; 9+ messages in thread
From: zimoun @ 2021-12-03 10:29 UTC (permalink / raw)
  To: Danny Milosavljevic; +Cc: 34170, contact

Hi,

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

--8<---------------cut here---------------start------------->8---
$ 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
--8<---------------cut here---------------end--------------->8---

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

1: <http://issues.guix.gnu.org/issue/34170>


Cheers,
simon




^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#34170: bitcoin-core bundles leveldb
  2021-12-03 10:29 ` zimoun
@ 2022-01-04 23:29   ` zimoun
  2022-01-05  9:39   ` Maxime Devos
  1 sibling, 0 replies; 9+ messages in thread
From: zimoun @ 2022-01-04 23:29 UTC (permalink / raw)
  To: Danny Milosavljevic; +Cc: 34170-done, contact

Hi,

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

> 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




^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#34170: bitcoin-core bundles leveldb
  2021-12-03 10:29 ` zimoun
  2022-01-04 23:29   ` zimoun
@ 2022-01-05  9:39   ` Maxime Devos
  2022-01-05  9:45     ` zimoun
  1 sibling, 1 reply; 9+ messages in thread
From: Maxime Devos @ 2022-01-05  9:39 UTC (permalink / raw)
  To: zimoun, Danny Milosavljevic; +Cc: 34170, contact

[-- Attachment #1: Type: text/plain, Size: 1247 bytes --]

zimoun schreef op vr 03-12-2021 om 11:29 [+0100]:
> 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#34170: bitcoin-core bundles leveldb
  2022-01-05  9:39   ` Maxime Devos
@ 2022-01-05  9:45     ` zimoun
  2022-01-05 17:40       ` Carl Dong
  0 siblings, 1 reply; 9+ messages in thread
From: zimoun @ 2022-01-05  9:45 UTC (permalink / raw)
  To: Maxime Devos; +Cc: 34170, contact

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




^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#34170: bitcoin-core bundles leveldb
  2022-01-05  9:45     ` zimoun
@ 2022-01-05 17:40       ` Carl Dong
  2022-01-05 19:13         ` Maxime Devos
  0 siblings, 1 reply; 9+ messages in thread
From: Carl Dong @ 2022-01-05 17:40 UTC (permalink / raw)
  To: zimoun; +Cc: 34170

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

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

> 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 #2: Type: text/html, Size: 3224 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#34170: bitcoin-core bundles leveldb
  2022-01-05 17:40       ` Carl Dong
@ 2022-01-05 19:13         ` Maxime Devos
  2022-01-07  0:17           ` Carl Dong
  0 siblings, 1 reply; 9+ messages in thread
From: Maxime Devos @ 2022-01-05 19:13 UTC (permalink / raw)
  To: Carl Dong, zimoun; +Cc: 34170

[-- Attachment #1: Type: text/plain, Size: 2999 bytes --]

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 #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#34170: bitcoin-core bundles leveldb
  2022-01-05 19:13         ` Maxime Devos
@ 2022-01-07  0:17           ` Carl Dong
  2022-07-13 15:10             ` Maxim Cournoyer
  0 siblings, 1 reply; 9+ messages in thread
From: Carl Dong @ 2022-01-07  0:17 UTC (permalink / raw)
  To: Maxime Devos; +Cc: 34170


[-- Attachment #1.1: Type: text/plain, Size: 4356 bytes --]

> 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>

> 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


> 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 #1.2: Type: text/html, Size: 6656 bytes --]

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* bug#34170: bitcoin-core bundles leveldb
  2022-01-07  0:17           ` Carl Dong
@ 2022-07-13 15:10             ` Maxim Cournoyer
  0 siblings, 0 replies; 9+ messages in thread
From: Maxim Cournoyer @ 2022-07-13 15:10 UTC (permalink / raw)
  To: Carl Dong; +Cc: 34170, Danny Milosavljevic, Maxime Devos, zimoun

Hi Carl,

Carl Dong <contact@carldong.me> writes:

>> 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>
>
>> 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

Would you like to give the idea discussed above a try and send a patch?
That'd bring closure to this old issue :-).

Thanks,

Maxim




^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-07-13 15:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-22 12:31 bug#34170: bitcoin-core bundles leveldb Danny Milosavljevic
2021-12-03 10:29 ` zimoun
2022-01-04 23:29   ` zimoun
2022-01-05  9:39   ` Maxime Devos
2022-01-05  9:45     ` zimoun
2022-01-05 17:40       ` Carl Dong
2022-01-05 19:13         ` Maxime Devos
2022-01-07  0:17           ` Carl Dong
2022-07-13 15:10             ` Maxim Cournoyer

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).