* [Bitcoin-development] LevelDB in master
@ 2013-08-16 9:39 Luke-Jr
2013-08-16 9:52 ` Peter Todd
0 siblings, 1 reply; 3+ messages in thread
From: Luke-Jr @ 2013-08-16 9:39 UTC (permalink / raw)
To: Bitcoin Dev
[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]
Now-merged pull request #2702 appears to have put the master branch on an
unofficial Ripple fork of LevelDB, rather than merely updating us to LevelDB
1.12.0. While Vinnie did somewhat disclose this, I don't see any evidence the
nature of this was fully understood by others. As I understood the pull
request, the "Ripple and Bitcoin fork" was just LevelDB with the changes we
had already made. Mike's comments on the pull request (his audit) suggest that
this may have been the case in an earlier revision of it. But in fact, there
appear to be a number of other changes included in what was finally merged a
few weeks ago. Furthermore, Ripple's fork did not do a proper git merge of
upstream, thus there is a break in git history, and, more importantly, a
number of upstream fixes (including some we have had reported to the Bitcoin
issue tracker) were not included in this merge.
I've pushed three branches to https://github.com/luke-jr/leveldb :
bitcoin-1.5 Our old/unreleased LevelDB 1.5 fork, for reference
bitcoin Our LevelDB 1.7 fork, included in 0.8.x
bitcoin-up Our LevelDB 1.7 fork, merged with upstream LevelDB 1.12
A diff from current master (Ripple LevelDB 1.12 fork) to bitcoin-up:
https://gist.github.com/luke-jr/6248543
Thoughts?
Luke
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 1530 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Bitcoin-development] LevelDB in master
2013-08-16 9:39 [Bitcoin-development] LevelDB in master Luke-Jr
@ 2013-08-16 9:52 ` Peter Todd
2013-08-17 20:53 ` Pieter Wuille
0 siblings, 1 reply; 3+ messages in thread
From: Peter Todd @ 2013-08-16 9:52 UTC (permalink / raw)
To: Luke-Jr; +Cc: Bitcoin Dev
[-- Attachment #1: Type: text/plain, Size: 1860 bytes --]
On Fri, Aug 16, 2013 at 09:39:16AM +0000, Luke-Jr wrote:
> Now-merged pull request #2702 appears to have put the master branch on an
> unofficial Ripple fork of LevelDB, rather than merely updating us to LevelDB
> 1.12.0. While Vinnie did somewhat disclose this, I don't see any evidence the
> nature of this was fully understood by others. As I understood the pull
> request, the "Ripple and Bitcoin fork" was just LevelDB with the changes we
> had already made. Mike's comments on the pull request (his audit) suggest that
> this may have been the case in an earlier revision of it. But in fact, there
> appear to be a number of other changes included in what was finally merged a
> few weeks ago. Furthermore, Ripple's fork did not do a proper git merge of
> upstream, thus there is a break in git history, and, more importantly, a
> number of upstream fixes (including some we have had reported to the Bitcoin
> issue tracker) were not included in this merge.
>
> I've pushed three branches to https://github.com/luke-jr/leveldb :
> bitcoin-1.5 Our old/unreleased LevelDB 1.5 fork, for reference
> bitcoin Our LevelDB 1.7 fork, included in 0.8.x
> bitcoin-up Our LevelDB 1.7 fork, merged with upstream LevelDB 1.12
>
> A diff from current master (Ripple LevelDB 1.12 fork) to bitcoin-up:
> https://gist.github.com/luke-jr/6248543
>
> Thoughts?
I ran into this problem while auditing Litecoin actually: the tools to
audit that a set of git patches/merges actually match upstream (or
downstream for litecoin) don't really exist yet. In this case manually
checking that individual files matched would have probably worked, but
it'd be good to automate the process.
I can't say I've looked into any of this in detail, but you're right to
bring up the issue.
--
'peter'[:-1]@petertodd.org
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Bitcoin-development] LevelDB in master
2013-08-16 9:52 ` Peter Todd
@ 2013-08-17 20:53 ` Pieter Wuille
0 siblings, 0 replies; 3+ messages in thread
From: Pieter Wuille @ 2013-08-17 20:53 UTC (permalink / raw)
To: Peter Todd; +Cc: Bitcoin Dev
On Fri, Aug 16, 2013 at 11:52 AM, Peter Todd <pete@petertodd•org> wrote:
> On Fri, Aug 16, 2013 at 09:39:16AM +0000, Luke-Jr wrote:
>> Now-merged pull request #2702 appears to have put the master branch on an
>> unofficial Ripple fork of LevelDB, rather than merely updating us to LevelDB
>> 1.12.0. While Vinnie did somewhat disclose this, I don't see any evidence the
>> nature of this was fully understood by others. As I understood the pull
>> request, the "Ripple and Bitcoin fork" was just LevelDB with the changes we
>> had already made. Mike's comments on the pull request (his audit) suggest that
>> this may have been the case in an earlier revision of it. But in fact, there
>> appear to be a number of other changes included in what was finally merged a
>> few weeks ago. Furthermore, Ripple's fork did not do a proper git merge of
>> upstream, thus there is a break in git history, and, more importantly, a
>> number of upstream fixes (including some we have had reported to the Bitcoin
>> issue tracker) were not included in this merge.
>>
>> I've pushed three branches to https://github.com/luke-jr/leveldb :
>> bitcoin-1.5 Our old/unreleased LevelDB 1.5 fork, for reference
>> bitcoin Our LevelDB 1.7 fork, included in 0.8.x
>> bitcoin-up Our LevelDB 1.7 fork, merged with upstream LevelDB 1.12
>>
>> A diff from current master (Ripple LevelDB 1.12 fork) to bitcoin-up:
>> https://gist.github.com/luke-jr/6248543
Thanks for investigating this. I guess it's my fault for not checking
the diff before the final merge. I guess the simultaneous switch to a
git-subtree'd leveldb made it harder to review.
In any case, the changes seem harmless, but I think we should revert
to a codebase as close as possible to upstream LevelDB 1.12. The diff
you have between bitcoin head and bitcoin-up shows a few reverted
patches that we included during 0.9's merge window, a patch by ripple
to add a compaction delay (which they seem to have reverted now too)
and one weird ripple-specific commit (which just seems to remove
issue178_test.cc).
I've put a cleaned-up history of the LevelDB subtree in the
http://github.com/bitcoin/leveldb repository (branch bitcoin-fork),
and then used git-subtree to create a pull request (#2907) which
switches our src/leveldb directory to this tree. It correctly lists
the reverted (and sometimes re-applied) changes in the squashed commit
(please review!). The actual diff corresponds to the diff you
produced, with the reverted changes in our repository re-applied.
--
Pieter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-08-17 20:53 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-16 9:39 [Bitcoin-development] LevelDB in master Luke-Jr
2013-08-16 9:52 ` Peter Todd
2013-08-17 20:53 ` Pieter Wuille
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox