-
Notifications
You must be signed in to change notification settings - Fork 36k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove txindex migration code #22626
Conversation
Concept ACK I'd suggest maybe adding a release note to inform users about this change, as was done in #13033 when |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Concept ACK |
Concept ACK |
faa605b
to
fa78a79
Compare
I don't think this needs a release note because txindex is off by default and the number of txindex users upgrading from 0.16 (or earlier) to 23.x (or later) while reading the release notes, should be non-existent. Also for them, the targeted error message will explain what is going on. |
Concept ACK |
1 similar comment
Concept ACK |
Concept ACK. |
Approach ACK |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK fa78a79.
fa78a79
to
fa20f81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK fa20f81, tested on Linux Mint 20.2 (x86_64).
The old-fashion txindex was created by Bitcoin Core 0.16.3.
This PR, the first run:
...
2021-08-20T22:20:22Z init message: Verifying blocks…
2021-08-20T22:20:23Z Verifying last 6 blocks at level 3
2021-08-20T22:20:23Z [0%]...[16%]...[33%]...[50%]...[66%]...[83%]...[99%]...[DONE].
2021-08-20T22:20:23Z No coin database inconsistencies in last 6 blocks (288 transactions)
2021-08-20T22:20:23Z block index 20085ms
2021-08-20T22:20:23Z Error: The block index db contains a legacy 'txindex'. To clear the occupied disk space, run a full -reindex, otherwise ignore this error. This error message will not be displayed again.
Error: The block index db contains a legacy 'txindex'. To clear the occupied disk space, run a full -reindex, otherwise ignore this error. This error message will not be displayed again.
2021-08-20T22:20:23Z Shutdown: In progress...
2021-08-20T22:20:23Z scheduler thread exit
2021-08-20T22:20:27Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
2021-08-20T22:20:27Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
2021-08-20T22:20:30Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
2021-08-20T22:20:30Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
2021-08-20T22:20:31Z Shutdown: done
No txindex related error messages during the following runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK fa20f81
LGTM 🧉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK fa20f81
Found some more includes / forward declarations in src/validation.h
that seem unused and hence could be removed in the first commit:
#include <crypto/common.h> // for ReadLE64
#include <protocol.h> // For CMessageHeader::MessageStartChars
class CBlockUndo;
class CInv;
class CConnman;
Might do in the next force push or a new pull, whichever happens earlier. |
I'm not sure we care, but what if a user upgrades to 23.0, syncs further a bit, then downgrades back to an older version that uses this txindex? Will it silently be broken for txindex purposes? Will it fail to sync properly (perhaps if an expected txindex entry is missing)? |
The txindex will be disabled for older version as soon as you start with a new version. See also the comment: // Disable legacy txindex and warn once about occupied disk space |
Code review ACK fa20f81 |
fadc4c7 test: Add txindex migration test (MarcoFalke) Pull request description: Test for bitcoin#22626 ACKs for top commit: theStack: Tested ACK fadc4c7 🌁 Tree-SHA512: fc7133ef52826bf0d4fa2ac72c3f1bed4a185ff7492396552ff2cbf6531b053238039211a710cbb949379c56875cd7715f1ed49a514dd3b3f1b46554e3d4bef5
fadc4c7 test: Add txindex migration test (MarcoFalke) Pull request description: Test for bitcoin#22626 ACKs for top commit: theStack: Tested ACK fadc4c7 🌁 Tree-SHA512: fc7133ef52826bf0d4fa2ac72c3f1bed4a185ff7492396552ff2cbf6531b053238039211a710cbb949379c56875cd7715f1ed49a514dd3b3f1b46554e3d4bef5
It was put there thinking it would be needed by the test harness and suites, which work fine without it, and it must be removed before API test harness can run against bitcoin-core v23. See bitcoin/bitcoin#22626 (comment)
This change needs thorough review before being merged. See bitcoin-core Issue "Remove txindex migration code #22626": bitcoin/bitcoin#22626 And comment about change not having a release note: bitcoin/bitcoin#22626 (comment) See src: https://github.com/bitcoin/bitcoin/blob/v23.0/src/txdb.cpp#L35 For API test suites: This change is backward compatible for bitcoin-core v22. This change has not been tested against bitcoin-core v21, v20, v19. Change needs to be thoroughly tested with desktop UI.
This method hasn't been used since the txindex migration code has been removed (PR bitcoin#22626, commit fa20f81). Co-authored-by: furszy <matiasfurszyfer@protonmail.com>
…es `CDBIterator,CDBWrapper,CCoinsViewDBCursor` e4b4db5 refactor: remove unused method `CDBWrapper::CompactRange` (Sebastian Falbesoner) fb38c6e refactor: remove unused methods `{CDBIterator,CCoinsViewDBCursor}::GetValueSize()` (Sebastian Falbesoner) Pull request description: The `GetValueSize` methods haven't been used since the chainstate db cache has been switched from per-tx to per-txout model years ago (PR #10195, commit d342424). The `CompactRange` is unused since the txindex migration code was removed (PR bitcoin/bitcoin#22626, commit bitcoin/bitcoin@fa20f81). ACKs for top commit: fanquake: ACK e4b4db5 furszy: re-ACK e4b4db5 laanwj: Code review ACK e4b4db5 Tree-SHA512: 77da445fb70c744046263c6f2ddb05782b68e3d4b2ea604dd7c7dc73ce7c1f2d2b48ec68db4dcb03e35fc27488b99b0a420f6fa3d5b83d325c1708ed68e99e0a
…terator,CDBWrapper,CCoinsViewDBCursor` e4b4db5 refactor: remove unused method `CDBWrapper::CompactRange` (Sebastian Falbesoner) fb38c6e refactor: remove unused methods `{CDBIterator,CCoinsViewDBCursor}::GetValueSize()` (Sebastian Falbesoner) Pull request description: The `GetValueSize` methods haven't been used since the chainstate db cache has been switched from per-tx to per-txout model years ago (PR bitcoin#10195, commit d342424). The `CompactRange` is unused since the txindex migration code was removed (PR bitcoin#22626, commit bitcoin@fa20f81). ACKs for top commit: fanquake: ACK e4b4db5 furszy: re-ACK e4b4db5 laanwj: Code review ACK e4b4db5 Tree-SHA512: 77da445fb70c744046263c6f2ddb05782b68e3d4b2ea604dd7c7dc73ce7c1f2d2b48ec68db4dcb03e35fc27488b99b0a420f6fa3d5b83d325c1708ed68e99e0a
Summary: > Add missing includes and forward declarations, remove unused ones > doc: Fix validation typo > Remove txindex migration code PR description: > No supported version of Bitcoin used the legacy txindex, so all relevant nodes can be assumed to have upgraded. Thus, there is no need to keep this code any longer. > > As a temporary courtesy, provide a one-time warning on how to free the disk space used by the legacy txindex. This is a backport of [[bitcoin/bitcoin#22626 | core#22626]] Depends on D12242 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12244
No supported version of Bitcoin Core used the legacy txindex, so all relevant nodes can be assumed to have upgraded. Thus, there is no need to keep this code any longer.
As a temporary courtesy, provide a one-time warning on how to free the disk space used by the legacy txindex.
Fixes #22615