Remove txindex migration code #22626

pull MarcoFalke wants to merge 3 commits into bitcoin:master from MarcoFalke:2108-noTxindexMigrate changing 8 files +44 −175
  1. MarcoFalke commented at 8:14 pm on August 4, 2021: member

    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

  2. Zero-1729 commented at 8:33 pm on August 4, 2021: contributor

    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.

  3. DrahtBot commented at 9:12 pm on August 4, 2021: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #22951 (consensus: move amount.h into consensus by fanquake)
    • #22242 (Move CBlockTreeDB to node/blockstorage by MarcoFalke)
    • #15606 (assumeutxo by jamesob)

    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.

  4. practicalswift commented at 9:13 pm on August 4, 2021: contributor
    Concept ACK
  5. DrahtBot added the label UTXO Db and Indexes on Aug 4, 2021
  6. DrahtBot added the label Validation on Aug 4, 2021
  7. jnewbery commented at 9:59 pm on August 4, 2021: member
    Concept ACK
  8. MarcoFalke force-pushed on Aug 5, 2021
  9. MarcoFalke commented at 7:07 am on August 5, 2021: member
    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.
  10. MarcoFalke removed the label Validation on Aug 5, 2021
  11. theStack commented at 11:55 am on August 5, 2021: member
    Concept ACK
  12. jamesob commented at 2:16 pm on August 5, 2021: member
    Concept ACK
  13. fanquake added this to the milestone 23.0 on Aug 12, 2021
  14. in src/txdb.cpp:48 in fa78a79a51 outdated 
    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.");
    laanwj commented at 2:31 pm on August 18, 2021:
    What will happen if the user does not do a -reindex in this case? Will it automatically start a partial reindex next time?
    MarcoFalke commented at 5:29 pm on August 18, 2021:
    There will be an “unreachable” txindex in the block index db eating your disk space. It shouldn’t have any behavioural impact otherwise.
    laanwj commented at 5:42 pm on September 16, 2021:
    Thanks, that seems fine.
  15. in src/txdb.cpp:39 in fa78a79a51 outdated 
    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.");
    laanwj commented at 2:32 pm on August 18, 2021:
    Maybe txindex “upgrade” instead of “migration”, I would imagine that word could confuse translators.
    MarcoFalke commented at 3:00 pm on August 20, 2021:
    thanks, done
  16. hebasto commented at 1:31 pm on August 20, 2021: member
    Concept ACK.
  17. in src/txdb.h:24 in fa78a79a51 outdated 
    22-class CCoinsViewDBCursor;
23 class uint256;
24+namespace Consensus {
25+struct Params;
26+};
27+struct bilingual_str;
    hebasto commented at 1:43 pm on August 20, 2021:

    fa11e29f1addb291352c0185e2e2be960ba7c66c

    Add class COutPoint;?

    MarcoFalke commented at 3:01 pm on August 20, 2021:
    forgot to do this. Might do on the next re-push.
  18. in src/validation.h:14 in fa78a79a51 outdated 
    10@@ -11,7 +11,9 @@
11 #endif
12 
13 #include <amount.h>
14+#include <arith_uint256.h>
    hebasto commented at 1:51 pm on August 20, 2021:

    fa11e29f1addb291352c0185e2e2be960ba7c66c

    Add #include <uint256.h>?

    MarcoFalke commented at 3:01 pm on August 20, 2021:
    thx, done
  19. in src/validation.h:16 in fa78a79a51 outdated 
    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>
    hebasto commented at 1:53 pm on August 20, 2021:

    fa11e29f1addb291352c0185e2e2be960ba7c66c

    Now the class CBlockIndex; forward declaration looks unneeded.

    MarcoFalke commented at 3:02 pm on August 20, 2021:
    thx, done
  20. Zero-1729 commented at 2:06 pm on August 20, 2021: contributor
    Approach ACK
  21. hebasto commented at 2:15 pm on August 20, 2021: member
    Approach ACK fa78a79a51c9a621fd7fbf9f2cb7b4348f3f6119.
  22. Add missing includes and forward declarations, remove unused ones fab89006d6
  23. doc: Fix validation typo fae8786033
  24. Remove txindex migration code fa20f815a9
  25. MarcoFalke force-pushed on Aug 20, 2021
  26. hebasto approved
  27. hebasto commented at 10:27 pm on August 20, 2021: member

    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:

     0...
 12021-08-20T22:20:22Z init message: Verifying blocks…
 22021-08-20T22:20:23Z Verifying last 6 blocks at level 3
 32021-08-20T22:20:23Z [0%]...[16%]...[33%]...[50%]...[66%]...[83%]...[99%]...[DONE].
 42021-08-20T22:20:23Z No coin database inconsistencies in last 6 blocks (288 transactions)
 52021-08-20T22:20:23Z  block index           20085ms
 62021-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.
 7Error: 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.
 82021-08-20T22:20:23Z Shutdown: In progress...
 92021-08-20T22:20:23Z scheduler thread exit
102021-08-20T22:20:27Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
112021-08-20T22:20:27Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
122021-08-20T22:20:30Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) started
132021-08-20T22:20:30Z FlushStateToDisk: write coins cache to disk (0 coins, 0kB) completed (0.00s)
142021-08-20T22:20:31Z Shutdown: done

    No txindex related error messages during the following runs.

  28. Zero-1729 commented at 6:10 am on August 21, 2021: contributor

    crACK fa20f815a9cb438c5ab61e97a453612ddd8b21b5

    LGTM 🧉

  29. fanquake requested review from jnewbery on Sep 2, 2021
  30. fanquake requested review from theStack on Sep 2, 2021
  31. fanquake requested review from jamesob on Sep 2, 2021
  32. theStack approved
  33. theStack commented at 8:16 pm on September 2, 2021: member

    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::MessageStartChars
    • class CBlockUndo;
    • class CInv;
    • class CConnman;
  34. MarcoFalke commented at 8:01 am on September 3, 2021: member

    Found some more includes

    Might do in the next force push or a new pull, whichever happens earlier.

  35. luke-jr commented at 4:32 pm on September 12, 2021: member
    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)?
  36. MarcoFalke commented at 11:10 am on September 13, 2021: member

    The txindex will be disabled for older version as soon as you start with a new version. See also the comment:

    0         // Disable legacy txindex and warn once about occupied disk space
  37. laanwj commented at 5:44 pm on September 16, 2021: member
    Code review ACK fa20f815a9cb438c5ab61e97a453612ddd8b21b5
  38. laanwj merged this on Sep 16, 2021
  39. laanwj closed this on Sep 16, 2021
  40. sidhujag referenced this in commit 9577a9811b on Sep 19, 2021
  41. MarcoFalke deleted the branch on Sep 20, 2021
  42. MarcoFalke referenced this in commit 41a1b5f58c on Nov 15, 2021
  43. sidhujag referenced this in commit a79ed4fd02 on Nov 15, 2021
  44. ghubstan referenced this in commit 0b66c2f9a2 on May 19, 2022
  45. ghubstan referenced this in commit 3fe72b7425 on May 21, 2022
  46. theStack referenced this in commit e4b4db5610 on Jun 23, 2022
  47. MarcoFalke referenced this in commit f697c068eb on Jun 24, 2022
  48. sidhujag referenced this in commit e1fc0f67a5 on Jun 27, 2022
  49. Fabcien referenced this in commit 10ea90c31d on Oct 17, 2022
  50. DrahtBot locked this on Oct 30, 2022


MarcoFalke Zero-1729 DrahtBot practicalswift jnewbery theStack jamesob laanwj hebasto luke-jr


jamesob jnewbery

Labels
UTXO Db and Indexes

Milestone
23.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-18 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me