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
Concept ACK
I'd suggest maybe adding a release note to inform users about this change, as was done in #13033 when txindex was moved. Maybe let them know about the removal and needing to use -reindex now. It could be added to this PR or a follow-up.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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
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
Concept ACK
43 | + if (txindex_legacy_flag) { 44 | + // Disable legacy txindex and warn once about occupied disk space 45 | + if (!block_tree_db.WriteFlag("txindex", false)) { 46 | + return Untranslated("Failed to write block index db flag 'txindex'='0'"); 47 | + } 48 | + return _("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.");
What will happen if the user does not do a -reindex in this case? Will it automatically start a partial reindex next time?
There will be an "unreachable" txindex in the block index db eating your disk space. It shouldn't have any behavioural impact otherwise.
Thanks, that seems fine.
34 | + 35 | +std::optional<bilingual_str> CheckLegacyTxindex(CBlockTreeDB& block_tree_db) 36 | +{ 37 | + CBlockLocator ignored{}; 38 | + if (block_tree_db.Read(DB_TXINDEX_BLOCK, ignored)) { 39 | + return _("The -txindex migration started by a previous version can not be completed. Restart with the previous version or run a full -reindex.");
Maybe txindex "upgrade" instead of "migration", I would imagine that word could confuse translators.
thanks, done
Concept ACK.
22 | -class CCoinsViewDBCursor; 23 | class uint256; 24 | +namespace Consensus { 25 | +struct Params; 26 | +}; 27 | +struct bilingual_str;
fa11e29f1addb291352c0185e2e2be960ba7c66c
Add class COutPoint;?
forgot to do this. Might do on the next re-push.
10 | @@ -11,7 +11,9 @@ 11 | #endif 12 | 13 | #include <amount.h> 14 | +#include <arith_uint256.h>
fa11e29f1addb291352c0185e2e2be960ba7c66c
Add #include <uint256.h>?
thx, done
10 | @@ -11,7 +11,9 @@ 11 | #endif 12 | 13 | #include <amount.h> 14 | +#include <arith_uint256.h> 15 | #include <attributes.h> 16 | +#include <chain.h>
fa11e29f1addb291352c0185e2e2be960ba7c66c
Now the class CBlockIndex; forward declaration looks unneeded.
thx, done
Approach ACK
Approach ACK fa78a79a51c9a621fd7fbf9f2cb7b4348f3f6119.
ACK fa20f815a9cb438c5ab61e97a453612ddd8b21b5, 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.
crACK fa20f815a9cb438c5ab61e97a453612ddd8b21b5
LGTM 🧉
Approach ACK fa20f815a9cb438c5ab61e97a453612ddd8b21b5
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::MessageStartCharsclass CBlockUndo;class CInv;class CConnman;
Found some more includes
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 fa20f815a9cb438c5ab61e97a453612ddd8b21b5
Milestone
23.0