assumeutxo #15606

pull jamesob wants to merge 26 commits into bitcoin:master from jamesob:utxo-dumpload-compressed changing 38 files +1290 −238
  1. jamesob commented at 1:58 pm on March 15, 2019: member

    See the proposal for assumeutxo here.

    Testing instructions can be found below the “Progress” section.


    Progress

    All items here have corresponding commits here, but are unchecked if they haven’t been merged yet.

    • Chainstate interface
    • Localize chainstate data
    • Share block data
    • Deglobalize chainstate
    • UpdateTip/CheckBlockIndex modifications
    • ChainstateManager
    • Mempool
    • LoadBlockIndex
    • Init/teardown
    • Wallet: includes avoiding rescans when assumed-valid block data is in use
    • P2P: minor changes are made to init.cpp and net_processing.cpp to make simultaneous IBD across multiple chainstates work.
    • Pruning: implement correct pruning behavior when using a background chainstate
    • Blockfile separation: to prevent “fragmentation” in blockfile storage, have background chainstates use separate blockfiles from active snapshot chainstates to avoid interleaving heights and impairing pruning.
    • Indexing: all existing CValidationInterface events are given with an additional parameter, ChainstateRole, and all indexers ignore events from ChainstateRole::ASSUMEDVALID so that indexation only happens sequentially.
    • Raise error when both -reindex and assumeutxo are in use.
    • RPC: introduce RPC commands dumptxoutset, loadtxoutset, and (the probably temporary) monitorsnapshot.
    • Release docs & first assumeutxo commitment: add notes and a particular assumeutxo hash value for first AU-enabled release.
      • This will complete the project and allow use of UTXO snapshots for faster node bootstrap.
    • (optional) Coinscache optimization: allow flushing chainstate data without emptying the coins cache; results in better performance after UTXO snapshot load.

    Testing

    For fun (~5min)

    If you want to do a quick test, you can run ./contrib/devtools/test_utxo_snapshots.sh and follow the instructions. This is mostly obviated by the functional tests, though.

    For real (longer)

    If you’d like to experience a real usage of assumeutxo, you can do that too. I’ve cut a new snapshot at height 788'000 (http://img.jameso.be/utxo-788000.dat - but you can do it yourself with ./contrib/devtools/utxo_snapshot.sh if you want). Download that, and then create a datadir for testing:

     0$ cd ~/src/bitcoin  # or whatever
     1
     2# get the snapshot
     3$ curl http://img.jameso.be/utxo-788000.dat > utxo-788000.dat
     4
     5# you'll want to do this if you like copy/pasting 
     6$ export AU_DATADIR=/home/${USER}/au-test # or wherever
     7
     8$ mkdir ${AU_DATADIR}
     9$ vim ${AU_DATADIR}/bitcoin.conf
    10
    11dbcache=8000  # or, you know, something high
    12blockfilterindex=1
    13coinstatsindex=1
    14prune=3000
    15logthreadnames=1
    

    Obtain this branch, build it, and then start bitcoind:

    0$ git remote add jamesob https://github.com/jamesob/bitcoin
    1$ git fetch jamesob utxo-dumpload-compressed
    2$ git checkout jamesob/utxo-dumpload-compressed
    3
    4$ ./configure $conf_args && make  # (whatever you like to do here)
    5
    6# start 'er up and watch the logs
    7$ ./src/bitcoind -datadir=${AU_DATADIR}
    

    Then, in some other window, load the snapshot

    0$ ./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset $(pwd)/utxo-788000.dat
    

    You’ll see some log messages about headers retrieval and waiting to see the snapshot in the headers chain. Once you get the full headers chain, you’ll spend a decent amount of time (~10min) loading the snapshot, checking it, and flushing it to disk. After all that happens, you should be syncing to tip in pretty short order, and you’ll see the occasional [background validation] log message go by.

    In yet another window, you can check out chainstate status with

    0$ ./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
    

    as well as usual favorites like getblockchaininfo.


    Original change description

    For those unfamiliar with assumeutxo, here’s a brief summary from the issue (where any conceptual discussion not specific to this implementation should happen):

    assumeutxo would be a way to initialize a node using a headers chain and a serialized version of the UTXO state which was generated from another node at some block height. A client making use of this UTXO “snapshot” would specify a hash and expect the content of the resulting UTXO set to yield this hash after deserialization.

    This would allow users to bootstrap a usable pruned node & wallet far more quickly (and with less disk usage) than waiting for a full initial block download to complete, since we only have to sync blocks between the base of the snapshot and the current network tip. Needless to say this is at expense of accepting a different trust model, though how different this really ends up being from assumevalid in effect is worth debate.

    In short, this is an interesting change because it would allow nodes to get up and running within minutes given a ~3GB file (at time of writing) under an almost identical trust model to assumevalid.

    In this implementation, I add a few RPC commands: dumptxoutset creates a UTXO snapshot and writes it to disk, and loadtxoutset intakes a snapshot from disk, constructs and activates chainstate based on it, and continues a from-scratch initial block download in the background for the sole purpose of validating the snapshot. Once the snapshot is validated, we throw away the chainstate used for background validation.

    The assumeutxo procedure as implemented is as follows:

    1. A UTXO snapshot is loaded with the loadtxoutset <path> RPC command.
    2. A new chainstate (CChainState) is initialized using ChainstateManager::ActivateSnapshot():
      1. The serialized UTXO data is read in and various sanity checks are performed, e.g. compare expected coin count, recompute the hash and compare it with assumeutxo hash in source code.
      2. We “fast forward” new_chainstate->m_chain to have a tip at the base of the snapshot (with or without block data). Lacking block data, we fake the nTx counts of the constituent CBlockIndex entries.
      3. LoadChainTip() is called on the new snapshot and it is installed as our active chainstate.
    3. The new assumed-valid chainstate is now our active, and so that enters IBD until it is synced to the network’s tip. Presumably the snapshot would be taken relatively close to the current tip but far enough away to avoid meaningful reorgs, say 10,000 blocks deep.
    4. Once the active chainstate is out of IBD, our old validation chain continues IBD “in the background” while the active chainstate services requests from most of the system.
    5. Once the background validation chainstate reaches a height equal the base of the snapshot, we take the hash of its UTXO set and ensure it equals the expected hash based on the snapshot. If the hashes are equivalent, we delete the validation chainstate and move on without event; if they aren’t, we log loudly and fall back to the validation chainstate (we should probably just shut down).

    The implicit assumption is that the background validation chain will always be a subset of the assumed-valid snapshot chain while the latter is active. We don’t properly handle reorgs that go deeper than the base of the snapshot.

    Changes (already merged/outdated)

    chainstate-beforeafter (1)

    The crux of this change is in removing any assumptions in the codebase that there is a single chainstate, i.e. any references to global variables chainActive, pcoinsTip, et al. need to be replaced with functions that return the relevant chainstate data at that moment in time. This change also takes CChainState to its logical conclusion by making it more self-contained - any references to globals like chainActive are removed with class-local references (m_chain).

    A few minor notes on the implementation:

    • When we attempt to load a wallet with a BestBlock locator lower than the base of a snapshot and the snapshot has not yet been validated, we refuse to load the wallet.

    • For additional notes, see the new assumeutxo docs.

  2. fanquake added the label UTXO Db and Indexes on Mar 15, 2019
  3. fanquake added the label Validation on Mar 15, 2019
  4. DrahtBot commented at 3:07 pm on March 15, 2019: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK MarcoFalke, Sjors
    Approach ACK ryanofsky

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27491 (refactor: Move chain constants to the util library by TheCharlatan)
    • #27357 (validation: Move warningcache to ChainstateManager and rename to m_warningcache by dimitaracev)
    • #27277 (Move log messages: tx enqueue to mempool, allocation to blockstorage by Sjors)
    • #27125 (refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan)
    • #27039 (blockstorage: do not flush block to disk if it is already there by pinheadmz)
    • #26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
    • #26762 (refactor: Make CCheckQueue RAII-styled by hebasto)
    • #25977 (refactor: Replace std::optional<bilingual_str> with util::Result by ryanofsky)
    • #25970 (Add headerssync tuning parameters optimization script to repo by sipa)
    • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
    • #25665 (refactor: Add util::Result failure values, multiple error and warning messages by ryanofsky)
    • #25193 (indexes: Read the locator’s top block during init, allow interaction with reindex-chainstate by mzumsande)
    • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
    • #24008 (assumeutxo: net_processing changes 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.

  5. gmaxwell commented at 7:39 pm on March 15, 2019: contributor

    Presumably the snapshot would be taken relatively close to the current tip but far enough away to avoid meaningful reorgs, say 10,000 blocks deep.

    To be clear, the snapshot can’t be terribly recent for the user of it or it breaks the security model. Assume valid only has any security at all if there is time for the review and communication about review to happen, which means a human timescale of at least weeks.

    Probably the assumption should be that snapshots are created pretty close to tip, just far enough to avoid wasting time in reorgs (even 6 blocks would be fine) but the users aren’t using them until they are quite a bit older, likely using the second to most recent available one.

    We don’t serve blocks or transactions to the network while we’re operating with an unvalidated snapshot-based chain.

    We shouldn’t do that. The worst that getting it wrong does is getting us disconnected from peers, which isn’t particularly cosmic (and at least we might notice that something is going wrong). The downside of it is that not forwarding transactions shoots our privacy in the head, since any transaction we’re emitting would be one we created.

    When we attempt to load a wallet with a BestBlock locator lower than the base of a snapshot and the snapshot has not yet been validated, we refuse to load the wallet.

    Sounds good.

    compare it with the claimed hash in the snapshot metadata.

    This is a zero security approach. You can’t just ask the user for a value and then accept that. The attack is to reorg the chain and then announce to everyone that there is a node bug and you need to run this command to continue, we specifically engineered out that possibility in assumevalid. This isn’t hypothetical, this is exactly what we’ve seen happen in ethereum w/ fastsync.

    This is a fine setup for testing your PR however! Ultimately we should get it to a state where a (FEC-split) copy of the state is loaded from the network and where it can be tested against a publicly reviewed constants configured in the software and/or blockchain.

  6. jamesob commented at 2:11 pm on March 19, 2019: member

    Probably the assumption should be that snapshots are created pretty close to tip, just far enough to avoid wasting time in reorgs (even 6 blocks would be fine) but the users aren’t using them until they are quite a bit older, likely using the second to most recent available one.

    Yep - I figured that we’d update the assumeutxo hash in lockstep with assumevalid’s.

    We shouldn’t do that. The worst that getting it wrong does is getting us disconnected from peers, which isn’t particularly cosmic (and at least we might notice that something is going wrong). The downside of it is that not forwarding transactions shoots our privacy in the head […]

    Good points, will fix that.

    This is a zero security approach. You can’t just ask the user for a value and then accept that.

    Agreed - the check I do there is just for sanity. Deciding on an initial hardcoded assumeutxo hash seems corequisite to allowing snapshots to be loadable through RPC (or any other means).

  7. DrahtBot added the label Needs rebase on Mar 21, 2019
  8. jamesob force-pushed on Mar 21, 2019
  9. jamesob force-pushed on Mar 21, 2019
  10. DrahtBot removed the label Needs rebase on Mar 21, 2019
  11. jamesob force-pushed on Mar 21, 2019
  12. jamesob force-pushed on Mar 21, 2019
  13. jamesob referenced this in commit 99e5086461 on Mar 21, 2019
  14. ryanofsky commented at 7:39 pm on March 26, 2019: contributor
    There are so many code changes here, and it seems like 80% of them are just renames. I know you are putting off really breaking this up and restructuring it, but maybe you could start by just splitting b2a735d00d3c297cdfdd93609c12537497025ca1 in two commits: one that adds the new classes and function arguments and brute force renames without changing behavior, and a smaller one with the new functionality. That way reviewers could see the more interesting changes without having to go through all the mind-numbing renames.
  15. jamesob force-pushed on Mar 27, 2019
  16. jamesob referenced this in commit 6cc2c6fcf1 on Mar 27, 2019
  17. jamesob force-pushed on Mar 27, 2019
  18. jamesob referenced this in commit 338b6ccb1e on Mar 27, 2019
  19. jamesob force-pushed on Mar 28, 2019
  20. jamesob referenced this in commit a958d6568c on Mar 28, 2019
  21. jamesob referenced this in commit 8accb019d0 on Mar 28, 2019
  22. jamesob referenced this in commit 20d1104586 on Mar 29, 2019
  23. jamesob referenced this in commit 7aa0669d40 on Mar 29, 2019
  24. jamesob force-pushed on Mar 30, 2019
  25. jamesob force-pushed on Mar 30, 2019
  26. jamesob force-pushed on Mar 30, 2019
  27. jamesob commented at 1:41 am on March 30, 2019: member

    Thanks for the suggestion, @ryanofsky. I spent the last few days reconstructing the changeset into sensible commits - hopefully it’s easier to understand this way.

    Most of the early commits are just shuffling stuff around (though all of it strictly necessary AFAICT). I’ve phrased the changes as much as possible as scripted-diffs and move-onlys. In replaying the changes, I found a few unnecessary diffs to omit.

    If anyone has additional suggestions for how this could be made easier to review, I’m all ears, though I’m not holding my breath waiting until some Concept (N)ACKs roll in on #15605.

  28. jamesob force-pushed on Mar 30, 2019
  29. jamesob force-pushed on Mar 30, 2019
  30. jamesob force-pushed on Apr 1, 2019
  31. DrahtBot added the label Needs rebase on Apr 1, 2019
  32. jamesob force-pushed on Apr 1, 2019
  33. DrahtBot removed the label Needs rebase on Apr 1, 2019
  34. in src/validation.h:451 in 77e5c90216 outdated
    446+};
    447+
    448+class ConnectTrace;
    449+
    450+namespace {
    451+    struct CBlockIndexWorkComparator
    


    ryanofsky commented at 5:03 pm on April 1, 2019:

    In commit “move-only: make the CChainState interface public” (77e5c902168ca2191448c5f8924b6831dc2717b9)

    Better to drop anonymous namespace. It doesn’t actually hide any functionality and causes warnings:

    0./validation.h:487:7: warning: ‘CChainState’ has a field ‘CChainState::setBlockIndexCandidates’ whose type uses the anonymous namespace [-Wsubobject-linkage]
    

    maflcko commented at 7:22 pm on April 1, 2019:
    Does CBlockIndexWorkComparator have to be in the header or would it be sufficient to forward declare and put the implementation in the cpp file only?

    ryanofsky commented at 7:34 pm on April 1, 2019:
    It has to be in the header because it affects the layout of the CChainState object.
  35. in src/validation.h:617 in b5627060df outdated
    613@@ -614,6 +614,11 @@ void ResetBlockFailureFlags(CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_mai
    614 /** The currently-connected chain of blocks (protected by cs_main). */
    615 extern CChain& chainActive;
    616 
    617+extern CChainState g_chainstate;
    


    ryanofsky commented at 6:59 pm on April 1, 2019:

    In commit “refactoring: introduce unused ChainActive()” (b5627060df9c7876d91391e34c84a7e4fb1ff378)

    Line seems unrelated to this commit. Maybe move to commit that first uses this.


    maflcko commented at 5:55 pm on April 10, 2019:
    This hasn’t been fixed?

    jamesob commented at 6:10 pm on April 10, 2019:
    Sorry - I’m marking these resolved as I rebase locally as a checklist for myself. Instead of pushing each time I rebase, I’m going to push all the changes up once I’m done (today or tomorrow).
  36. in src/validation.cpp:63 in 612d1b5df5 outdated
    59@@ -60,6 +60,8 @@
    60 
    61 CChainState g_chainstate;
    62 
    63+CChainState* ChainstateActive() { return &g_chainstate; }
    


    ryanofsky commented at 7:10 pm on April 1, 2019:

    In commit “refactoring: introduce ChainstateActive()” (612d1b5df556eb70eb5fb670d166a1d7927e6efc)

    This is returning a pointer, but none of the places calling it are checking null, so it seems like it’d be better to return a reference instead. Also would make ChainstateActive() more consistent with ChainActive()

  37. ryanofsky commented at 7:15 pm on April 1, 2019: contributor

    Started review (will update list below with progress).

    • 77e5c902168ca2191448c5f8924b6831dc2717b9 move-only: make the CChainState interface public (1/39)
    • bd6c3c1fa50bed627345b25c14b1f1a824b82cd0 rename: CChainState.chainActive -> m_chain (2/39)
    • b5627060df9c7876d91391e34c84a7e4fb1ff378 refactoring: introduce unused ChainActive() (3/39)
    • 3b7184e514f9dac7949007a34147b43383c3d229 scripted-diff: replace chainActive -> ::ChainActive() (4/39)
    • 47f8e200e7159e6145f26fa711dceb19cf054a8f refactoring: remove unused chainActive (5/39)
    • 612d1b5df556eb70eb5fb670d166a1d7927e6efc refactoring: introduce ChainstateActive() (6/39)
    • c9d13d26314e2368562a1612bc0c89f3099c4154 refactoring: FlushStateToDisk -> CChainState (7/39)
    • e9fab103e6f81cf32b09618ea9a70540616f7da8 refactoring: IsInitialBlockDownload -> CChainState (8/39)
    • e8e09fcb16b7e358a88b4f38d0d8af49a3eafac3 move-onlyish: move CCoinsViewErrorCatcher out of init.cpp (9/39)
    • 155ee896520e51b8a13d6904e3a14b5fa50295c0 refactoring: move block metadata into BlockMetadataManager (10/39)
    • 525df322de8e6803f41a56aff16bfc1ebe95f845 refactoring: have CCoins* data managed under CChainState (11/39)
    • 56ad8d226539a44daef02f479fab1820c536731f move-only: move coins statistics utils out of RPC (12/39)
    • 0b33ff5bcba7f2aa357a1a12fe1e1452c23defb7 validationinterface: add unused CChainState parameter (13/39)
    • ebc844e8a1bacfe603db85ef4deae6e1f109cd57 chain: add unused CChain::FakeNTx (14/39)
    • 50d8e7c910edec10dcf08beec577c3efd67f2276 validation: add unused SnapshotMetadata class (15/39)
    • 7f39dfd011c6921b396c5acfc8917f748c1e97dd coinstats: add coins_count (16/39)
    • 71a65a08213b0159d4e4df2199d9993547f163e4 rpc: add dumptxoutset (17/39)
    • deca3be21d35ca661e047512f74c805ee24ecb16 refactoring: add CChainState::ActivateBestChain (18/39)
    • 3faddddba7abcdd609aae08653c35c05602a086f refactoring: move LoadChainTip to CChainState method (19/39)
    • d9505cd540bfa94d07c24ed7471822e1babdbfee coins: add constructor to CCoinsCacheEntry (20/39)
    • 10973e8e1dfd1951a98bf0de4c98b8cee385d70d trivial: add CChainState::ToString() (21/39)

    ~ 8d9b707f609b647fb28ae182a097adc03e718725 validation: introduce ChainstateManager, use in init (22/39) ~ 9b6907e080fe4da105a332c76ddb5110310e8840 validation: add ChainstateManager::ActivateSnapshot (23/39) ~ f9571218d41304040778655e2f3941042b358dd5 rpc: add loadtxoutset (24/39) ~ 6d85241c236f563715d4a982651fd1815d76dbd2 validation: introduce ChainstateManager::GetChainstateForNewBlock (25/39)

    • 6f87ecc366defcc9ce14ad9ec8ba0ae12652f866 net_processing: work with multiple chainstates (26/39)
    • da14524e5214462a67ea03519229fa789311343e refactoring: move ReplayBlocks under CChainState (27/39)
    • be40574dfaa13414881d16d9b82798af74026428 init: fix up for multiple chainstates (28/39)
    • 8f99b98b0a742b49614e26f572a6822516b0ae61 validation: fix up LoadBlockIndex for multiple chainstates (29/39)
    • 30ac0bb2a32f1dea8c2f7401168463adcf068137 dbwrapper: add delete-on-deconstruct option (30/39)
    • e037ea550056acef55d585b001ec7dd707ecd7be validation: add ChainstateManager logic for completing UTXO snapshot validation (31/39)
    • 32c054bd7b4300617ca4220edf5fdf3d203933a9 validation: put UTXO snapshot validation completion into practice (32/39)
    • a3e6f67bf547b76037bb6964a84647a6e3db19e2 wallet: avoid rescans if under the snapshot (33/39)
    • 0a1fd4881bf3d1a353c926ab7517159f0928567a doc: add note about snapshots and index building (34/39)
    • bced4f61eac00f9f3ed36f9d6a3c91c3013171fd validation: fix pruning + misc. (35/39)
    • 183d744247fb4b25edb28a1f2a3c7df7a00d8d87 doc: add some minor CChainState doc (36/39)
    • da94a6d64132b1d16fae7c275fe5b9fad0b7308c zmq: fix notifier to ignore background snapshot validation (37/39)
    • 5f24ef13a84356a79f9ce6e921f1a3afcce74313 rpc: add monitorsnapshot (38/39)
    • 1bb64fcf1cb94e28c4090909e229f546757d25de DON’T MERGE: add utxo snapshot demo script (39/39)
  38. in src/validation.cpp:201 in c9d13d2631 outdated
    163-    IF_NEEDED,
    164-    PERIODIC,
    165-    ALWAYS
    166-};
    167-
    168-// See definition for documentation
    


    ryanofsky commented at 6:16 pm on April 2, 2019:

    In commit “refactoring: FlushStateToDisk -> CChainState” (c9d13d26314e2368562a1612bc0c89f3099c4154)

    Could keep this comment, I think it applies to the whole block of function declarations.

  39. in src/validation.h:294 in c9d13d2631 outdated
    290@@ -290,9 +291,9 @@ void PruneOneBlockFile(const int fileNumber) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    291 void UnlinkPrunedFiles(const std::set<int>& setFilesToPrune);
    292 
    293 /** Flush all state, indexes and buffers to disk. */
    294-void FlushStateToDisk();
    295+void FlushStateToDisk(CChainState* chainstate = nullptr);
    


    ryanofsky commented at 6:31 pm on April 2, 2019:

    In commit “refactoring: FlushStateToDisk -> CChainState” (c9d13d26314e2368562a1612bc0c89f3099c4154)

    I think it’d be better not to have these default parameters and chain guessing code:

    0if (!chainstate) {
    1    chainstate = ::ChainstateActive();
    2}
    

    It seems like choosing a chain implicitly makes the call sites less straightforward, and increases the risk of a bug due to using the wrong chain.

    Also, I don’t think having these defaults arguments would be a good idea even if they made the diff smaller, but I think they might actually make it bigger, since these functions aren’t called very many places.

  40. in src/validation.h:547 in e9fab103e6 outdated
    542+     * Is this chainstate undergoing initial block download?
    543+     *
    544+     * Mutable because we need to be able to mark IsInitialBlockDownload()
    545+     * const, which toggles this for caching purposes.
    546+     */
    547+    mutable std::atomic<bool> m_is_ibd;
    


    ryanofsky commented at 9:33 pm on April 2, 2019:

    In commit “refactoring: IsInitialBlockDownload -> CChainState” (e9fab103e6f81cf32b09618ea9a70540616f7da8)

    Would call this m_cached_in_ibd or something to indicate the value may not be up to date. I could imagine incorrect code that checks m_in_ibd when it should be calling IsInitialBlockDownload instead. Calling this “cached” would make the bug more obvious.

    Could also update the comment above to suggest calling IsInitialBlockDownload instead of accessing this variable.


    ryanofsky commented at 8:09 pm on April 5, 2019:

    In commit “refactoring: IsInitialBlockDownload -> CChainState” (e9fab103e6f81cf32b09618ea9a70540616f7da8):

    Style guide prefers (and it’s a little clearer) to initialize variables like this where they’re declared, instead of separately in constructors. Could write m_is_ibd = false or m_is_ibd{false}

  41. in src/coins.cpp:277 in e8e09fcb16 outdated
    272+        // could lead to invalid interpretation. Just exit immediately, as we can't
    273+        // continue anyway, and all writes should be atomic.
    274+        std::abort();
    275+    }
    276+
    277+    // Writes do not need similar protection, as failure to write is handled by the caller.
    


    ryanofsky commented at 9:40 pm on April 2, 2019:

    In commit “move-onlyish: move CCoinsViewErrorCatcher out of init.cpp” (e8e09fcb16b7e358a88b4f38d0d8af49a3eafac3)

    I think this comment makes more sense in the place where it was written previously (in class definition rather than in GetCoin method implementation).

  42. in src/coins.h:329 in e8e09fcb16 outdated
    324+    typedef std::function<void()> Function;
    325+
    326+    explicit CCoinsViewErrorCatcher(CCoinsView* view) : CCoinsViewBacked(view) {}
    327+
    328+    void AddReadErrCallback(const Function f) {
    329+      m_err_callbacks.push_back(f);
    


    ryanofsky commented at 9:43 pm on April 2, 2019:

    In commit “move-onlyish: move CCoinsViewErrorCatcher out of init.cpp” (e8e09fcb16b7e358a88b4f38d0d8af49a3eafac3)

    Spacing seems off, maybe use clang-format for the new code.

  43. in src/coins.h:328 in e8e09fcb16 outdated
    323+public:
    324+    typedef std::function<void()> Function;
    325+
    326+    explicit CCoinsViewErrorCatcher(CCoinsView* view) : CCoinsViewBacked(view) {}
    327+
    328+    void AddReadErrCallback(const Function f) {
    


    ryanofsky commented at 9:48 pm on April 2, 2019:

    In commit “move-onlyish: move CCoinsViewErrorCatcher out of init.cpp” (e8e09fcb16b7e358a88b4f38d0d8af49a3eafac3)

    Not a big deal, but it would be nice to avoid copies:

    0void AddReadErrCallback(Function f) {
    1    m_err_callbacks.emplace_back(std::move(f));
    2}
    
  44. in src/validation.cpp:1212 in 155ee89652 outdated
    1208@@ -1148,7 +1209,7 @@ void static InvalidChainFound(CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(c
    1209 void CChainState::InvalidBlockFound(CBlockIndex *pindex, const CValidationState &state) {
    1210     if (!state.CorruptionPossible()) {
    1211         pindex->nStatus |= BLOCK_FAILED_VALID;
    1212-        m_failed_blocks.insert(pindex);
    1213+        g_blockmetaman.m_failed_blocks.insert(pindex);
    


    ryanofsky commented at 10:04 pm on April 2, 2019:

    In commit “refactoring: move block metadata into BlockMetadataManager” (155ee896520e51b8a13d6904e3a14b5fa50295c0)

    It seems like a step backwards to be replacing a local reference with a global reference. I get that BlockMetadataManager is a singleton shared between multiple chainstates, but I think it’s a mistake to add global references to it all over the code. If you could add a BlockMetadataManager& m_blockmetaman member to CChainState (also needs forward declaration and initialization in constructor), and replace g_blockmetaman with m_blockmetaman where possible, I think it would help make relationship between the classes clearer, and also be a step in the direction of making validation code more testable and possible to break apart.


    jamesob commented at 7:48 pm on April 10, 2019:
    Good advice, thanks. I’ve renamed the class BlockManager and folded the “global” instance into ChainstateManager, which then injects a reference to that instance when constructing chainstates. Each reference lives as CChainState.m_blockman.
  45. DrahtBot added the label Needs rebase on Apr 3, 2019
  46. in src/init.cpp:1521 in 525df322de outdated
    1504@@ -1501,33 +1505,23 @@ bool AppInitMain(InitInterfaces& interfaces)
    1505                 // At this point we're either in reindex or we've loaded a useful
    1506                 // block tree into mapBlockIndex!
    1507 
    1508-                pcoinsdbview.reset(new CCoinsViewDB(nCoinDBCache, false, fReset || fReindexChainState));
    


    ryanofsky commented at 7:41 pm on April 3, 2019:

    In commit “refactoring: have CCoins* data managed under CChainState” (525df322de8e6803f41a56aff16bfc1ebe95f845)

    I don’t see any problems with this change, but it seems strange that previous code called pcoinsdbview.reset() and pcoinscatcher.reset() above, and then reset them again here, when it could have just set them once above like this commit is doing now.

    Just asking for reassurance that previous code didn’t intentionally reset these twice, since now they’re only set once.

    I guess this is ok assuming fReset || fReindexChainState has the same value above?

  47. in src/init.cpp:229 in 525df322de outdated
    225@@ -228,7 +226,7 @@ void Shutdown(InitInterfaces& interfaces)
    226     }
    227 
    228     // FlushStateToDisk generates a ChainStateFlushed callback, which we should avoid missing
    229-    if (pcoinsTip != nullptr) {
    230+    if (::ChainstateActive()->CoinsCache() != nullptr) {
    


    ryanofsky commented at 7:52 pm on April 3, 2019:

    In commit “refactoring: have CCoins* data managed under CChainState” (525df322de8e6803f41a56aff16bfc1ebe95f845)

    Note: I was confused why init.cpp seemed to sometimes use ::ChainstateActive()-> and other places use g_chainstate. for apparently no reason. I think I would have preferred using g_chainstate. in the init.cpp file consistently and reserving ChainstateActive for network and validation code. But I think it doesn’t matter ultimately because g_chainstate is replaced in a later commit by g_chainman.

  48. in src/validation.h:669 in 525df322de outdated
    574      * missing the data for the block.
    575      */
    576     std::set<CBlockIndex*, CBlockIndexWorkComparator> setBlockIndexCandidates;
    577 
    578-    CBlockIndex *pindexBestInvalid = nullptr;
    579+    CCoinsViewCache* CoinsCache() { return m_coins_views->m_cacheview.get(); }
    


    ryanofsky commented at 8:04 pm on April 3, 2019:

    In commit “refactoring: have CCoins* data managed under CChainState” (525df322de8e6803f41a56aff16bfc1ebe95f845)

    Minor point, but I like the old name CoinsTip better than the new name CoinsCache. Just imagining seeing this code for the first time, I would expect the “db” object to be the object I should be reading and writing state to, and the “cache” object to be something used for optimization that I shouldn’t mess around with.

    In reality though, almost all code should be using the “cache” object and ignoring the “db” object, so I think “cache” is not a great name and “tip” would be better shorthand because this is the view you should be using if you care about data at the state of the tip.


    ryanofsky commented at 8:08 pm on April 3, 2019:

    In commit “refactoring: have CCoins* data managed under CChainState” (525df322de8e6803f41a56aff16bfc1ebe95f845)

    I think these three functions should be returning references instead of pointers, and maybe asserting the underlying unique_ptrs are set, since no callers are ever checking to see if the pointers are null.


    jamesob commented at 6:44 pm on April 9, 2019:
    Yep, agree with you. Will change.
  49. in src/coinstats.cpp:1 in 56ad8d2265 outdated
    0@@ -0,0 +1,73 @@
    1+// Copyright (c) 2019 The Bitcoin Core developers
    


    ryanofsky commented at 8:13 pm on April 3, 2019:

    In commit “move-only: move coins statistics utils out of RPC” (56ad8d226539a44daef02f479fab1820c536731f)

    Maybe preserve the original copyright right years if you are moving existing code to a new file.

  50. in src/validationinterface.h:91 in 0b33ff5bcb outdated
    86@@ -86,7 +87,11 @@ class CValidationInterface {
    87      *
    88      * Called on a background thread.
    89      */
    90-    virtual void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) {}
    91+    virtual void UpdatedBlockTip(
    92+        const CChainState* chainstate,
    


    ryanofsky commented at 8:19 pm on April 3, 2019:

    In commit “validationinterface: add unused CChainState parameter” (0b33ff5bcba7f2aa357a1a12fe1e1452c23defb7)

    Will these new chainstate parameters ever be null? Is any implementation ever going to check that they are not null before using them? Would suggest changing to const CChainState& chainstate to avoid mishaps otherwise.

  51. in src/chain.h:462 in ebc844e8a1 outdated
    458@@ -459,6 +459,9 @@ class CChain {
    459     /** Set/initialize a chain with a given tip. */
    460     void SetTip(CBlockIndex *pindex);
    461 
    462+    /** Ensure all members of this chain have a truthy nTx/nChainTx value. */
    


    ryanofsky commented at 8:24 pm on April 3, 2019:

    In commit “chain: add unused CChain::FakeNTx” (ebc844e8a1bacfe603db85ef4deae6e1f109cd57)

    Note: I don’t know if this comment is true, but it feels true.


    ryanofsky commented at 7:02 pm on April 4, 2019:

    re: #15606 (review)

    Now I’m confused about this comment. I thought it meant truthy in the Colbert sense (https://en.wikipedia.org/wiki/Truthiness). But that doesn’t work where truthy is used later in https://github.com/bitcoin/bitcoin/commit/8d9b707f609b647fb28ae182a097adc03e718725#diff-349fbb003d5ae550a2e8fa658e475880R777. Since this is C++, it would probably be clearer to say “non-zero” here and “non-null” there, if that’s what is meant.


    jamesob commented at 2:46 pm on April 8, 2019:
  52. in src/chain.cpp:28 in ebc844e8a1 outdated
    19@@ -20,6 +20,20 @@ void CChain::SetTip(CBlockIndex *pindex) {
    20     }
    21 }
    22 
    23+void CChain::FakeNTx(unsigned int nChainTx) {
    24+    for (CBlockIndex* index : vChain) {
    25+        if (!index->nTx) {
    26+            index->nTx = 1;
    27+        }
    28+        index->nChainTx = index->pprev ? index->pprev->nChainTx + index->nTx : 1;
    


    ryanofsky commented at 8:27 pm on April 3, 2019:

    In commit “chain: add unused CChain::FakeNTx” (ebc844e8a1bacfe603db85ef4deae6e1f109cd57)

    It seems really unfortunate that this is necessary just to report progress. Are there alternative ways to change the progress function so this would not be required?

  53. in src/validation.h:846 in 50d8e7c910 outdated
    671+    /**
    672+     * Necessary to "fake" the base nChainTx so that we can estimate progress during
    673+     * snapshot IBD.
    674+     */
    675+    unsigned int m_nchaintx = 0;
    676+    bool m_validation_complete = false;
    


    ryanofsky commented at 8:30 pm on April 3, 2019:

    In commit “validation: add unused SnapshotMetadata class” (50d8e7c910edec10dcf08beec577c3efd67f2276)

    Would add comments to describe these other fields.

    In particular, I’m curious why m_validation_complete would be a field that gets serialized as opposed to a memory-only state.


    jamesob commented at 7:05 pm on April 9, 2019:
    I’ll add comments, but to answer your immediate question: once we complete validation of a snapshot, we update this value to true and write it out to a metadata file on disk. This is so that when we are initializing after a subsequent restart, we know that the snapshot we load in has been validated and we don’t need to continue to do a background validation. This would be unnecessary if we renamed the chainstate_xxx...x leveldb directory used by the snapshot to chainstate after we complete background validation of the snapshot, which is maybe what we should do anyway.
  54. in src/rpc/blockchain.cpp:2297 in 71a65a0821 outdated
    2249+                RPCExamples{""},
    2250+            }.ToString()
    2251+        );
    2252+
    2253+    std::string path = request.params[0].get_str();
    2254+    FILE* file{fsbridge::fopen(path, "wb")};
    


    ryanofsky commented at 2:54 pm on April 4, 2019:

    In commit “rpc: add dumptxoutset” (71a65a08213b0159d4e4df2199d9993547f163e4)

    Do you want to allow overwriting existing files? If so, should include a comment saying it’s intentional, since this could be potentially dangerous (overwriting wallet files, etc). If not, should check fs::exists(path).

  55. in src/rpc/blockchain.cpp:2254 in 71a65a0821 outdated
    2248+                RPCResults{},
    2249+                RPCExamples{""},
    2250+            }.ToString()
    2251+        );
    2252+
    2253+    std::string path = request.params[0].get_str();
    


    ryanofsky commented at 3:00 pm on April 4, 2019:

    In commit “rpc: add dumptxoutset” (71a65a08213b0159d4e4df2199d9993547f163e4)

    Do you want to allow path to be relative? If not, should error when fs::path(path).is_relative(). If so, should probably interpret path relative to a known location like the datadir (fs::path path = fs::absolute(request.params[0].get_str(), GetDataDir())), instead of relative to whatever the current directory was when starting bitcoind or bitcoin-qt.

  56. in src/rpc/blockchain.cpp:2245 in 71a65a0821 outdated
    2239+{
    2240+    if (request.fHelp || request.params.size() != 1)
    2241+        throw std::runtime_error(
    2242+            RPCHelpMan{
    2243+                "dumptxoutset",
    2244+                "\nWrite the serialized UTXO set to disk.\n",
    


    ryanofsky commented at 3:01 pm on April 4, 2019:

    In commit “rpc: add dumptxoutset” (71a65a08213b0159d4e4df2199d9993547f163e4)

    Not sure, but it might be useful to mention if this flushes the cache.

  57. in src/rpc/blockchain.cpp:2684 in 71a65a0821 outdated
    2281+    while (pcursor->Valid()) {
    2282+        boost::this_thread::interruption_point();
    2283+        COutPoint key;
    2284+        Coin coin;
    2285+        if (pcursor->GetKey(key) && pcursor->GetValue(coin)) {
    2286+            afile << key;
    


    ryanofsky commented at 3:07 pm on April 4, 2019:

    In commit “rpc: add dumptxoutset” (71a65a08213b0159d4e4df2199d9993547f163e4)

    Can you add a comment about synchronization and atomicity here. It’s unclear why it’s safe to do all this without locking cs_main. Or is this assuming that another flush won’t happen in the background?

    Also, is it ok for this and GetUTXOStats to be using different cursor objects?


    jamesob commented at 3:02 pm on April 11, 2019:

    Yeah, this is an incorrect relic of this RPC being a hackneyed prototype. We’ll probably have to lock cs_main for the duration of this call for now, but I think that’ll be prohibitively lengthy when we start generating snapshots as part of normal operation, so down the road we may have to introduce a unique lock for the coinsdb (i.e. to prevent flushes from happening mid-snapshot).

    We’re fine to use different cursor objects so long as the leveldb data hasn’t changed between calls (which it obviously won’t if we hold cs_main).


    jamesob commented at 3:09 pm on April 11, 2019:
    This makes me think that the existing gettxoutsetinfo RPC is broken since it isn’t locking cs_main during the GetUTXOStats() call. I wonder if the author of that function assumed that leveldb handles locking as some kind of RAII associated with use of the iterator, but that’s not the case (DBImpl::NewIterator, DBIter).

    sipa commented at 3:20 pm on April 11, 2019:
    The cursor returned iterates over a snapshot of the database at the time of its creation; the db can be modified simultaneously, but won’t affect what the cursor sees.

    jamesob commented at 3:24 pm on April 11, 2019:
    @sipa I can’t find any mention of this behavior in the leveldb docs or header files. I see it happening here, but do we want to rely on something so implicit?

    sipa commented at 3:29 pm on April 11, 2019:
    We should probably make it explicit through the Snapshot API of LevelDB. I thought we already did, actually - so I’m a bit confused how I knew this as I don’t remember looking in the code.
  58. in src/rpc/blockchain.cpp:2295 in 71a65a0821 outdated
    2289+
    2290+        pcursor->Next();
    2291+    }
    2292+
    2293+    afile.fclose();
    2294+    return tfm::format("%d coins written at tip %s", stats.coins_count, tip->ToString());
    


    ryanofsky commented at 6:24 pm on April 4, 2019:

    In commit “rpc: add dumptxoutset” (71a65a08213b0159d4e4df2199d9993547f163e4)

    It is strange to return a formatted string from an RPC. Would seem better to return something structured like {"coins_written": stats.coins_count}

  59. in src/validation.h:695 in deca3be21d outdated
    601+      * Find the best known block, and make it the tip of the block chain
    602+      *
    603+      * May not be called with cs_main held. May not be called in a
    604+      * validationinterface callback.
    605+      */
    606+    bool ActivateBestChain(
    


    ryanofsky commented at 6:32 pm on April 4, 2019:

    In commit “refactoring: add CChainState::ActivateBestChain” (deca3be21d35ca661e047512f74c805ee24ecb16)

    Commit description is misleading. This commit is only documenting the method, not adding it. And I guess it is making the last block argument optional.

  60. in src/validation.h:713 in deca3be21d outdated
    595@@ -596,7 +596,16 @@ class CChainState {
    596         FlushStateMode mode,
    597         int nManualPruneHeight = 0);
    598 
    599-    bool ActivateBestChain(CValidationState &state, const CChainParams& chainparams, std::shared_ptr<const CBlock> pblock);
    600+     /**
    601+      * Find the best known block, and make it the tip of the block chain
    


    ryanofsky commented at 6:33 pm on April 4, 2019:

    In commit “refactoring: add CChainState::ActivateBestChain” (deca3be21d35ca661e047512f74c805ee24ecb16)

    It’s not clear from this description what the block argument does.

  61. in src/validation.cpp:5088 in 10973e8e1d outdated
    4592@@ -4593,6 +4593,14 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
    4593     assert(nNodes == forward.size());
    4594 }
    4595 
    4596+std::string CChainState::ToString()
    4597+{
    4598+    CBlockIndex* tip = m_chain.Tip();
    4599+    return tfm::format("Chainstate [%s] @ height %d (%s)",
    4600+        m_from_snapshot_blockhash.IsNull() ? "ibd" : "snapshot",
    


    ryanofsky commented at 6:41 pm on April 4, 2019:

    In commit “trivial: add CChainState::ToString()” (10973e8e1dfd1951a98bf0de4c98b8cee385d70d)

    m_from_snapshot_blockhash variable isn’t introduced yet, so this doesn’t compile

  62. in src/validation.cpp:114 in 155ee89652 outdated
    109+    void Unload() {
    110+        m_failed_blocks.clear();
    111+        m_blocks_unlinked.clear();
    112+        m_pindex_best_invalid = nullptr;
    113+
    114+        for (const BlockMap::value_type& entry : m_block_index) {
    


    ryanofsky commented at 6:48 pm on April 4, 2019:

    In commit “refactoring: move block metadata into BlockMetadataManager” (155ee896520e51b8a13d6904e3a14b5fa50295c0)

    This commit causes some lock annotation errors:

     0  CXX      libbitcoin_server_a-validation.o
     1validation.cpp:114:48: error: reading variable 'm_block_index' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
     2        for (const BlockMap::value_type& entry : m_block_index) {
     3                                               ^
     4validation.cpp:114:48: error: reading variable 'm_block_index' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
     5validation.cpp:118:9: error: reading variable 'm_block_index' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
     6        m_block_index.clear();
     7        ^
     8validation.cpp:3725:29: error: reading variable 'mapBlockIndex' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
     9    vSortedByHeight.reserve(mapBlockIndex.size());
    10                            ^
    11validation.cpp:3726:61: error: reading variable 'mapBlockIndex' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
    12    for (const std::pair<const uint256, CBlockIndex*>& item : mapBlockIndex)
    13                                                            ^
    14validation.cpp:3726:61: error: reading variable 'mapBlockIndex' requires holding mutex 'cs_main' [-Werror,-Wthread-safety-analysis]
    156 errors generated.
    16Makefile:7627: recipe for target 'libbitcoin_server_a-validation.o' failed
    

    jamesob commented at 7:22 pm on April 12, 2019:
    Fixed, thanks.
  63. in src/validation.h:899 in 8d9b707f60 outdated
    772+     */
    773+    std::unique_ptr<CChainState> m_ibd_chainstate GUARDED_BY(cs_main);
    774+
    775+    /**
    776+     * A chainstate initialized on the basis of a UTXO snapshot. If this is
    777+     * truthy, it is always our active chainstate unless proven invalid.
    


    ryanofsky commented at 7:04 pm on April 4, 2019:

    In commit “validation: introduce ChainstateManager, use in init” (8d9b707f609b647fb28ae182a097adc03e718725)

    Probably clearer to say “set” or “non-null” here instead of “truthy”.

  64. in src/validation.h:867 in 8d9b707f60 outdated
    740+ *
    741+ * This class provides abstractions that allow the retrieval of the current
    742+ * most-work chainstate ("Active") as well as chainstates which may be in
    743+ * background use to validate UTXO snapshots.
    744+ *
    745+ * This class was created to displace the use of global variables (e.g.
    


    ryanofsky commented at 7:08 pm on April 4, 2019:

    In commit “validation: introduce ChainstateManager, use in init” (8d9b707f609b647fb28ae182a097adc03e718725)

    Would drop this. Referencing historical variables in commit and review comments is helpful, but confusing in code comments. If this is supposed to suggest that there are still weird things about the code that could be cleaned up in the future, it would probably be better to say directly what the current problems are and how they might be addressed.

  65. in src/validation.h:926 in 8d9b707f60 outdated
    781+public:
    782+    /**
    783+     * Points to either the ibd or snapshot chainstate; indicates our
    784+     * most-work chain.
    785+     */
    786+    CChainState* m_active_chainstate GUARDED_BY(cs_main);
    


    ryanofsky commented at 7:12 pm on April 4, 2019:

    In commit “validation: introduce ChainstateManager, use in init” (8d9b707f609b647fb28ae182a097adc03e718725)

    Is this supposed to be public? Maybe mention why in comment if so.


    jamesob commented at 7:18 pm on April 12, 2019:
    Made it private and then friended the only two functions (ChainActive and ChainstateActive) which access it.
  66. in src/validation.h:817 in 8d9b707f60 outdated
    731@@ -711,14 +732,170 @@ class SnapshotMetadata
    732 
    733 };
    734 
    735+/**
    736+ * Provides an interface for creating and interacting with multiple
    


    ryanofsky commented at 7:15 pm on April 4, 2019:

    In commit “validation: introduce ChainstateManager, use in init” (8d9b707f609b647fb28ae182a097adc03e718725)

    Can you give a little overview of what is meant by the:

    • unready snapshot chainstate
    • ibd chainstate
    • snapshot chainstate
    • active chainstate
    • background validation chainstate
    • validated chainstate

    I am getting more than a little lost in the terminology, and it would nice to have it defined briefly in one place.


    ryanofsky commented at 9:00 pm on April 5, 2019:

    re: #15606 (review)

    Can you give a little overview of what is meant by the:

    • unready snapshot chainstate
    • ibd chainstate
    • snapshot chainstate
    • active chainstate
    • background validation chainstate
    • validated chainstate

    More chainstates from commit “validation: introduce ChainstateManager::GetChainstateForNewBlock” (6d85241c236f563715d4a982651fd1815d76dbd2):

    • most-work chainstate
    • validation chainstate (different from validated chainstate?)

    jamesob commented at 6:37 pm on April 11, 2019:
    Most of these are already documented with data docstrings, but I’ve added an overview in the classdoc.
  67. in src/validation.cpp:4883 in 9b6907e080 outdated
    4878+        size_t cache_size_bytes,
    4879+        bool in_memory)
    4880+{
    4881+    uint256 base_blockhash = metadata.m_base_blockheader.GetBlockHash();
    4882+
    4883+    // Important that we haven't set `m_snapshot_metadata` yet before this point.
    


    ryanofsky commented at 8:19 pm on April 5, 2019:

    In commit “validation: add ChainstateManager::ActivateSnapshot” (9b6907e080fe4da105a332c76ddb5110310e8840)

    It would seem better to drop the comment and just assert, or return an error if this is not the case.

  68. in src/validation.cpp:5600 in 9b6907e080 outdated
    4951+    }
    4952+
    4953+    int max_secs_to_wait_for_headers = 60 * 10;
    4954+    CBlockIndex* snapshot_start_block = nullptr;
    4955+
    4956+    while (max_secs_to_wait_for_headers > 0) {
    


    ryanofsky commented at 8:24 pm on April 5, 2019:

    In commit “validation: add ChainstateManager::ActivateSnapshot” (9b6907e080fe4da105a332c76ddb5110310e8840)

    Not sure, but probably should use interruption_point.

  69. in src/validation.h:1005 in 9b6907e080 outdated
    834+     *     faking nTx* block index data along the way.
    835+     *
    836+     *   - Move the new chainstate to `m_snapshot_chainstate` and make it our
    837+     *     ChainstateActive().
    838+     *
    839+     *   - Clear the mempool.
    


    ryanofsky commented at 8:30 pm on April 5, 2019:

    In commit “validation: add ChainstateManager::ActivateSnapshot” (9b6907e080fe4da105a332c76ddb5110310e8840)

    Can the comment say more about this? Will there even be anything in the mempool? Would it make sense to only drop incompatible transactions?

  70. in src/validation.cpp:4884 in 9b6907e080 outdated
    4879+        bool in_memory)
    4880+{
    4881+    uint256 base_blockhash = metadata.m_base_blockheader.GetBlockHash();
    4882+
    4883+    // Important that we haven't set `m_snapshot_metadata` yet before this point.
    4884+    CChainState* snapshot_chainstate = InitializeChainstate(
    


    ryanofsky commented at 8:37 pm on April 5, 2019:

    In commit “validation: add ChainstateManager::ActivateSnapshot” (9b6907e080fe4da105a332c76ddb5110310e8840):

    I don’t understand why m_unready_snapshot_chainstate is a class member. It seems like it should just be a local variable auto snapshot = MakeUnique<CChainState>(...) in this function that gets swapped in with m_snapshot_chainstate when it is ready below. This would simplify the class and InitializeChainstate.


    jamesob commented at 6:41 pm on April 11, 2019:
    Yep. This function used to be split across two (one of which was executed in a separate thread), so this is an artifact of that. Willfix.
  71. in src/validation.h:976 in 9b6907e080 outdated
    836+     *   - Move the new chainstate to `m_snapshot_chainstate` and make it our
    837+     *     ChainstateActive().
    838+     *
    839+     *   - Clear the mempool.
    840+     */
    841+    bool ActivateSnapshot(
    


    ryanofsky commented at 8:40 pm on April 5, 2019:

    In commit “validation: add ChainstateManager::ActivateSnapshot” (9b6907e080fe4da105a332c76ddb5110310e8840):

    Maybe add NODISCARD since this returns error on failure.

  72. in src/rpc/blockchain.cpp:2354 in f9571218d4 outdated
    2313+    FILE* file{fsbridge::fopen(path, "rb")};
    2314+    CAutoFile afile{file, SER_DISK, CLIENT_VERSION};
    2315+    SnapshotMetadata metadata;
    2316+    afile >> metadata;
    2317+
    2318+    g_chainman.ActivateSnapshot(
    


    ryanofsky commented at 8:42 pm on April 5, 2019:

    In commit “rpc: add loadtxoutset” (f9571218d41304040778655e2f3941042b358dd5)

    Should throw an exception on failure.

  73. in src/rpc/blockchain.cpp:2338 in f9571218d4 outdated
    2294@@ -2294,6 +2295,32 @@ UniValue dumptxoutset(const JSONRPCRequest& request)
    2295     return tfm::format("%d coins written at tip %s", stats.coins_count, tip->ToString());
    2296 }
    2297 
    2298+UniValue loadtxoutset(const JSONRPCRequest& request)
    2299+{
    2300+    if (request.fHelp || request.params.size() != 1)
    2301+        throw std::runtime_error(
    2302+            RPCHelpMan{"dumptxoutset",
    


    ryanofsky commented at 8:43 pm on April 5, 2019:

    In commit “rpc: add loadtxoutset” (f9571218d41304040778655e2f3941042b358dd5)

    Should say loadtxoutset

  74. in src/rpc/blockchain.cpp:2728 in f9571218d4 outdated
    2308+                RPCExamples{""},
    2309+            }.ToString()
    2310+        );
    2311+
    2312+    std::string path{request.params[0].get_str()};
    2313+    FILE* file{fsbridge::fopen(path, "rb")};
    


    ryanofsky commented at 8:45 pm on April 5, 2019:

    In commit “rpc: add loadtxoutset” (f9571218d41304040778655e2f3941042b358dd5)

    Like #15606 (review), should probably intepret path relative to datadir

  75. in src/rpc/blockchain.cpp:2380 in f9571218d4 outdated
    2316+    afile >> metadata;
    2317+
    2318+    g_chainman.ActivateSnapshot(
    2319+        &afile, metadata, ::ChainstateActive()->m_coins_cache_size_bytes, false);
    2320+
    2321+    return tfm::format("Loaded snapshot %s", metadata.m_base_blockheader.GetBlockHash().ToString());
    


    ryanofsky commented at 8:46 pm on April 5, 2019:

    In commit “rpc: add loadtxoutset” (f9571218d41304040778655e2f3941042b358dd5)

    Like #15606 (review), should probably return json information instead of human readable string.

  76. in src/validation.cpp:5017 in 6d85241c23 outdated
    5009@@ -5002,6 +5010,16 @@ bool ChainstateManager::ActivateSnapshot(
    5010     return true;
    5011 }
    5012 
    5013+CChainState* ChainstateManager::GetChainstateForNewBlock(const uint256& blockhash)
    5014+{
    5015+    if (m_snapshot_chainstate &&
    5016+            !m_snapshot_chainstate->m_chain.Contains(LookupBlockIndex(blockhash))) {
    5017+        return m_snapshot_chainstate.get();
    


    ryanofsky commented at 9:05 pm on April 5, 2019:

    In commit “validation: introduce ChainstateManager::GetChainstateForNewBlock” (6d85241c236f563715d4a982651fd1815d76dbd2)

    Like other places, callers never seem to be checking for null. This should just use references and return *m_snapshot_chainstate instead of m_snapshot_chainstate.get().

  77. in src/validation.cpp:5190 in 1bb64fcf1c outdated
    5024+    LogPrintf("[snapshot] loading coins from snapshot %s\n", base_blockhash.ToString());
    5025+
    5026+    while (coins_left > 0) {
    5027+        *coins_file >> outpoint;
    5028+        *coins_file >> coin;
    5029+        coins_map[outpoint] = CCoinsCacheEntry(std::move(coin), CCoinsCacheEntry::DIRTY);
    


    jamesob commented at 4:13 pm on April 9, 2019:
    Note: this needs to be rewritten to avoid assuming that the user has enough free memory to fit the entire snapshot. Right now it’ll OOM on machines with less than 3.2GB mem available.
  78. in src/rpc/blockchain.cpp:2399 in 1bb64fcf1c outdated
    2306+                },
    2307+                RPCResults{},
    2308+                RPCExamples{""},
    2309+            }.ToString()
    2310+        );
    2311+
    


    jamesob commented at 4:15 pm on April 9, 2019:
    TODO: should add a check here that the user hasn’t loaded any wallets with a bestblock height of less than the base of the snapshot.
  79. in src/validation.h:22 in 1bb64fcf1c outdated
    17@@ -18,7 +18,11 @@
    18 #include <protocol.h> // For CMessageHeader::MessageStartChars
    19 #include <script/script_error.h>
    20 #include <sync.h>
    21+#include <txmempool.h>
    


    maflcko commented at 4:10 pm on April 10, 2019:

    in commit 77e5c902168ca2191448c5f8924b6831dc2717b9:

    I’d prefer not to include a header when a forward declaration is enough.

    See the following fixup:

     0diff --git a/src/validation.cpp b/src/validation.cpp
     1index 4676a446f8..a67cd03686 100644
     2--- a/src/validation.cpp
     3+++ b/src/validation.cpp
     4@@ -22,6 +22,7 @@
     5 #include <policy/policy.h>
     6 #include <policy/rbf.h>
     7 #include <pow.h>
     8+#include <txmempool.h>
     9 #include <primitives/block.h>
    10 #include <primitives/transaction.h>
    11 #include <random.h>
    12diff --git a/src/validation.h b/src/validation.h
    13index feb92b3774..9ff65e2dce 100644
    14--- a/src/validation.h
    15+++ b/src/validation.h
    16@@ -18,7 +18,6 @@
    17 #include <protocol.h> // For CMessageHeader::MessageStartChars
    18 #include <script/script_error.h>
    19 #include <sync.h>
    20-#include <txmempool.h>
    21 #include <versionbits.h>
    22 
    23 #include <algorithm>
    24@@ -44,7 +43,7 @@ class CBlockPolicyEstimator;
    25 class CTxMemPool;
    26 class CValidationState;
    27 struct ChainTxData;
    28-
    29+struct DisconnectedBlockTransactions;
    30 struct PrecomputedTransactionData;
    31 struct LockPoints;
    32 
    
  80. maflcko commented at 5:59 pm on April 10, 2019: member

    Just some more comments.

    In commit 3b7184e514f9dac7949007a34147b43383c3d229:

    The scripted diff should not modify on-disc files that are not in git.

    Can be solved by using

    0sed --args ... $(git grep -l 'what_to_search_for')
    
  81. jamesob force-pushed on Apr 10, 2019
  82. jamesob force-pushed on Apr 10, 2019
  83. jamesob force-pushed on Apr 11, 2019
  84. jamesob force-pushed on Apr 12, 2019
  85. jamesob force-pushed on Apr 12, 2019
  86. jamesob force-pushed on Apr 12, 2019
  87. jamesob force-pushed on Apr 12, 2019
  88. jamesob force-pushed on Apr 12, 2019
  89. DrahtBot removed the label Needs rebase on Apr 15, 2019
  90. DrahtBot added the label Needs rebase on Apr 16, 2019
  91. jamesob force-pushed on Apr 16, 2019
  92. jamesob force-pushed on Apr 17, 2019
  93. DrahtBot removed the label Needs rebase on Apr 17, 2019
  94. jamesob force-pushed on Apr 17, 2019
  95. DrahtBot added the label Needs rebase on Apr 18, 2019
  96. Sjors commented at 6:12 pm on April 19, 2019: member

    I’m unable to compile this on macOS. See error log. It does compile on Ubuntu, despite quite a few warnings.

    I tested making a dump for testnet and then importing it with a fresh datadir. But I think I’m using it wrong. First time I tried to load it I got. That was before it fetched any headers. I waited for it to fetch a few blocks and tried again, but same error:

    0bitcoind: txdb.cpp:104: virtual bool CCoinsViewDB::BatchWrite(CCoinsMap&, const uint256&): Assertion `!hashBlock.IsNull()' failed.
    

    I don’t think an RPC method is the right tool for loading a snapshot (it’s fine for saving); probably better to just specify a location at launch.

    Can you add an actual assumeutxo value (mainnet & testnet)? When combined with #13713 it should be easier to create and test the snapshot (I had some difficulty wrapping my head around the demo script). That also means loadtxoutset can refuse to process a snapshot for a lower block height than the most recent fully processed block (even if the header is a valid-fork).

    To facilitate writing functional tests, you could allow a -assumeutxo param for regtest only.

  97. jamesob commented at 1:18 pm on April 23, 2019: member

    I’m unable to compile this on macOS. See error log. It does compile on Ubuntu, despite quite a few warnings.

    Thanks; could you post the warnings you’re receiving on Ubuntu? I’m not getting any warnings during compilation when using clang 9.0.0 and gcc 7.3.0 on Ubuntu 18.04.2.

    I don’t think an RPC method is the right tool for loading a snapshot (it’s fine for saving); probably better to just specify a location at launch.

    Loading snapshot only on startup would simplify things in the short run, but ultimately if we’re going to share snapshots over the P2P network, we’ll need to support loading them at runtime. Better to test that process out earlier IMO.

    I waited for it to fetch a few blocks and tried again, but same error…

    Oops, looks like I introduced a bug during a recent rebase. Fixing now. Thanks for testing!

    Can you add an actual assumeutxo value (mainnet & testnet)?

    Yep, I’ll do this in the next day or so.

    To facilitate writing functional tests, you could allow a -assumeutxo param for regtest only.

    Yep, this seems like a good idea. I’m also going to try to do as much unittesting as I can (as opposed to functional tests).

  98. jamesob force-pushed on Apr 23, 2019
  99. jamesob force-pushed on Apr 23, 2019
  100. DrahtBot removed the label Needs rebase on Apr 23, 2019
  101. jamesob force-pushed on Apr 23, 2019
  102. DrahtBot added the label Needs rebase on Apr 23, 2019
  103. jamesob force-pushed on Apr 23, 2019
  104. jamesob force-pushed on Apr 23, 2019
  105. DrahtBot removed the label Needs rebase on Apr 23, 2019
  106. jamesob force-pushed on Apr 25, 2019
  107. jamesob force-pushed on Apr 25, 2019
  108. jamesob force-pushed on Apr 25, 2019
  109. jamesob commented at 8:01 pm on April 25, 2019: member
    I’ve added particular assumeutxo values and a more narrow devtools script. @Sjors hopefully this should now build without trouble or warnings on MacOS, though I haven’t tested.
  110. jamesob commented at 6:35 pm on April 26, 2019: member
    When (and if) I get a few Concept ACKs on this, I’ll start to break this into smaller PRs with accompanying tests. Let me know if there’s anything that would make conceptual review easier.
  111. maflcko commented at 7:03 pm on April 26, 2019: member
    Concept ACK
  112. Sjors commented at 2:20 pm on April 27, 2019: member

    Concept ACK. It compiles now. I was able to create a snapshot using the script and load it on a fresh datadir. Though it crashed when I stopped the node and started it again:

    02019-04-27T14:18:10Z [snapshot] resetting coinsviews for Chainstate [ibd] @ height 410343 (000000000008b5afee8bdf7bbc4386c001615bbfcfe12e7ddd0ac19ab9a03235)
    12019-04-27T14:18:10Z [snapshot] resetting coinsviews for Chainstate [snapshot] @ height 1512817 (0000000000000332aba2ad56296b67b7b6664a46b1c23bb014bb9aad6fb20828)
    22019-04-27T14:18:13Z [default wallet] Releasing wallet
    32019-04-27T14:18:13Z Shutdown: done
    4
    5...
    62019-04-27T14:19:11Z Initializing chainstate Chainstate [snapshot] @ height -1 (null)
    7Assertion failed: (!setBlockIndexCandidates.empty()), function PruneBlockIndexCandidates, file validation.cpp, line 2401.
    

    It’s hard to follow what’s going on when loading the snapshot on a fresh node because the logs are flooded by the regular IBD in progress. Consider pausing IBD while loading the file.

    Once we reach the tip and it starts syncing from genesis in the background, the log is too quiet. It’s only showing allocation of block files. It would be nice to have a message for each block along the lines of UpdateChain: processed block 00000000000000a9d.... at height=1234 . Something less verbose, like on message every 1000 blocks, would also be fine. Otherwise it might end up snowing under important events at the tip.

    getblockchaininfo should contain fields to measure the progress of catching up in the background (and there needs to be something for the GUI to access, that can wait).

    How do plan to handle block pruning? Do we just split -prune in half while catchup is in progress? Do we double the minimum prune setting (when used with this feature)?

  113. DrahtBot added the label Needs rebase on Apr 30, 2019
  114. jamesob referenced this in commit 90e3e50de3 on May 3, 2019
  115. jamesob force-pushed on May 3, 2019
  116. jamesob referenced this in commit bbd47efbc9 on May 3, 2019
  117. n1bor commented at 11:05 am on May 5, 2019: none

    Just an FYI - I had a working POC of this that would have worked fine with the chainstate hash in the source code: https://gist.github.com/n1bor/55b25d72cd3c24eef85f7e24d549ef7a

    One interesting thing is if you write a lot of data to a leveldb database that is mostly sorted by key (or at least big chunks are ) it is very efficient.

  118. maflcko referenced this in commit b2a6b02161 on May 7, 2019
  119. jamesob force-pushed on May 7, 2019
  120. DrahtBot removed the label Needs rebase on May 7, 2019
  121. jamesob commented at 5:23 pm on May 7, 2019: member

    @Sjors thanks very much for testing.

    it crashed when I stopped the node and started it again

    You found a bug in how the init process interacts with an assumed-valid chainstate. I needed to add nChainTx reconstruction to LoadBlockIndex() and make some allowances for the assumed-valid chain in RewindBlockIndex(). The details of the fix are mostly contained in https://github.com/bitcoin/bitcoin/pull/15606/commits/7d10a00daca80c6a8e68196ae7c016c1123d91fe, but we also now write BLOCK_OPT_WITNESS into the appropriate CBlockIndex entries during snapshot activation (https://github.com/bitcoin/bitcoin/pull/15606/commits/7bfbb78b445efbefa5238b4d23290ad4d4db05cf#diff-24efdb00bfbe56b140fb006b562cc70bR5001).

    Consider pausing IBD while loading the file.

    On your advice I’m now acquiring cs_main during snapshot activation for clarity when testing, but I think that closer to merge we should undo this. The snapshot chainstate can populate on a separate thread while traditional IBD starts and I don’t think either slows the other down since they’re running on separate threads and writing to separate data structures (the block index excluded).

    Note that you can isolate most of the snapshot-related logs with tail -f debug.log | grep '[snapshot]'.

    Once we reach the tip and it starts syncing from genesis in the background, the log is too quiet.

    I’ve added a [background validation] UpdateTip: ... message for every 2000 blocks synced in the background.

    How do plan to handle block pruning?

    Since we don’t expect (or support handling) reorgs in the background validation chainstate, we prune it aggressively (within 1 block of tip; see https://github.com/bitcoin/bitcoin/pull/15606/commits/e37372a0236b12dff422eaeca643db7d64f58b48#diff-24efdb00bfbe56b140fb006b562cc70bR3635). Given current pruning precision, this seems like a rounding error and so I think should be sufficient.

    Thanks again for testing. The most recent version of this PR is rebased and should be usable.

  122. jamesob commented at 5:27 pm on May 7, 2019: member

    getblockchaininfo should contain fields to measure the progress of catching up in the background (and there needs to be something for the GUI to access, that can wait).

    You can currently get a sense of this with the (maybe temporary? but definitely poorly named) monitorsnapshot RPC. Suggestions on how to roll this into getblockchaininfo are welcome. Maybe a background: key that contains a nested version of the same information?

  123. in src/coinstats.h:15 in d49425d82f outdated
    10+#include <chain.h>
    11+#include <uint256.h>
    12+#include <sync.h>
    13+
    14+extern CCriticalSection cs_main;
    15+CBlockIndex* LookupBlockIndex(const uint256& blockhash);
    


    maflcko commented at 5:50 pm on May 7, 2019:
    0In file included from validation.cpp:12:
    1./coinstats.h:15:14: redundant redeclaration of ‘CBlockIndex* LookupBlockIndex(const uint256&)’ in same scope [-Wredundant-decls]
    
  124. Sjors commented at 6:17 pm on May 7, 2019: member

    Consider pausing IBD while loading the file. acquiring cs_main during snapshot activation for clarity when testing I don’t think either slows the other down since they’re running on separate threads

    It’s not about performance, but about being able to debug. I think we should keep this in for at least one release, because it’s easier than the grep approach. It could also make sense to wait ~60 seconds after the snapshot is loaded before resuming the original IBD. That way the log looks like:

    • IBD…
    • rpc call to load snapshot (if RPC debug is enabled)
    • pauze IBD
    • loading snapshot
    • snapshot loaded, begin sync from snapshot
    • little bit of sync from snapshot
    • IBD resumed
    • interwoven log entries from both syncs

    we prune it aggressively (within 1 block of tip)

    Pruning normally flushes dbcache though, are you preventing that?

    Maybe a background: key that contains a nested version of the same information?

    That seems better (or snapshot), since otherwise you have to call monitorsnapshot to even find out anything is happening.

  125. in src/validation.h:429 in e2c878bea3 outdated
    494+      * ahead and mark descendants of invalid blocks as FAILED_CHILD at that time,
    495+      * instead of putting things in this set.
    496+      */
    497+    std::set<CBlockIndex*> m_failed_blocks;
    498+
    499+public:
    


    maflcko commented at 10:08 pm on May 7, 2019:

    In commit e2c878bea3 refactoring: move block metadata structures into BlockManager

    style-nit: Duplicate public

    Also, wouldn’t it be clearer to not expose the block index via CChainState::BlockIndex? For me this is confusing because there is one blockindex, but several chainstates that could use it.

    I suggest the following diff, which adds a BlockManager::BlockIndex method (diff is on top of the current pull request, not the commit where I left this comment):

      0diff --git a/src/init.cpp b/src/init.cpp
      1index 5097a81f08..2235db5ba1 100644
      2--- a/src/init.cpp
      3+++ b/src/init.cpp
      4@@ -1563,7 +1563,7 @@ bool AppInitMain(InitInterfaces& interfaces)
      5 
      6                 // If the loaded chain has a wrong genesis, bail out immediately
      7                 // (we're likely using a testnet datadir, or the other way around).
      8-                if (!::ChainstateActive().BlockIndex().empty() &&
      9+                if (!g_chainman.BlockIndex().empty() &&
     10                         !LookupBlockIndex(chainparams.GetConsensus().hashGenesisBlock)) {
     11                     return InitError(_("Incorrect or no genesis block found. Wrong datadir for network?"));
     12                 }
     13@@ -1798,7 +1798,7 @@ bool AppInitMain(InitInterfaces& interfaces)
     14     //// debug print
     15     {
     16         LOCK(cs_main);
     17-        LogPrintf("m_block_index.size() = %u\n", ::ChainstateActive().BlockIndex().size());
     18+        LogPrintf("m_block_index.size() = %u\n", g_chainman.BlockIndex().size());
     19         chain_active_height = ::ChainActive().Height();
     20     }
     21     LogPrintf("nBestHeight = %d\n", chain_active_height);
     22diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
     23index 6cdd11233a..d8e40a5d56 100644
     24--- a/src/rpc/blockchain.cpp
     25+++ b/src/rpc/blockchain.cpp
     26@@ -1378,7 +1378,7 @@ static UniValue getchaintips(const JSONRPCRequest& request)
     27     std::set<const CBlockIndex*> setOrphans;
     28     std::set<const CBlockIndex*> setPrevs;
     29 
     30-    for (const std::pair<const uint256, CBlockIndex*>& item : ::ChainstateActive().BlockIndex())
     31+    for (const std::pair<const uint256, CBlockIndex*>& item : g_chainman.BlockIndex())
     32     {
     33         if (!::ChainActive().Contains(item.second)) {
     34             setOrphans.insert(item.second);
     35diff --git a/src/validation.cpp b/src/validation.cpp
     36index f7884759ad..ebdd3ba1bc 100644
     37--- a/src/validation.cpp
     38+++ b/src/validation.cpp
     39@@ -1044,11 +1044,6 @@ bool CChainState::IsInitialBlockDownload() const
     40     return false;
     41 }
     42 
     43-BlockMap& CChainState::BlockIndex()
     44-{
     45-    return m_blockman.m_block_index;
     46-}
     47-
     48 CBlockIndex *pindexBestForkTip = nullptr, *pindexBestForkBase = nullptr;
     49 
     50 static void AlertNotify(const std::string& strMessage)
     51@@ -1695,8 +1690,8 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
     52         //  relative to a piece of software is an objective fact these defaults can be easily reviewed.
     53         // This setting doesn't force the selection of any particular chain but makes validating some faster by
     54         //  effectively caching the result of part of the verification.
     55-        BlockMap::const_iterator  it = BlockIndex().find(hashAssumeValid);
     56-        if (it != BlockIndex().end()) {
     57+        BlockMap::const_iterator  it = m_blockman.BlockIndex().find(hashAssumeValid);
     58+        if (it != m_blockman.BlockIndex().end()) {
     59             if (it->second->GetAncestor(pindex->nHeight) == pindex &&
     60                 pindexBestHeader->GetAncestor(pindex->nHeight) == pindex &&
     61                 pindexBestHeader->nChainWork >= nMinimumChainWork) {
     62@@ -2787,8 +2782,8 @@ bool CChainState::InvalidateBlock(CValidationState& state, const CChainParams& c
     63 
     64         // The resulting new best tip may not be in setBlockIndexCandidates anymore, so
     65         // add it again.
     66-        BlockMap::iterator it = BlockIndex().begin();
     67-        while (it != BlockIndex().end()) {
     68+        BlockMap::iterator it = m_blockman.BlockIndex().begin();
     69+        while (it != m_blockman.BlockIndex().end()) {
     70             if (it->second->IsValid(BLOCK_VALID_TRANSACTIONS) && it->second->HaveTxsDownloaded() && !setBlockIndexCandidates.value_comp()(it->second, m_chain.Tip())) {
     71                 setBlockIndexCandidates.insert(it->second);
     72             }
     73@@ -2815,8 +2810,8 @@ void CChainState::ResetBlockFailureFlags(CBlockIndex *pindex) {
     74     int nHeight = pindex->nHeight;
     75 
     76     // Remove the invalidity flag from this block and all its descendants.
     77-    BlockMap::iterator it = BlockIndex().begin();
     78-    while (it != BlockIndex().end()) {
     79+    BlockMap::iterator it = m_blockman.BlockIndex().begin();
     80+    while (it != m_blockman.BlockIndex().end()) {
     81         if (!it->second->IsValid() && it->second->GetAncestor(nHeight) == pindex) {
     82             it->second->nStatus &= ~BLOCK_FAILED_MASK;
     83             setDirtyBlockIndex.insert(it->second);
     84@@ -3956,7 +3951,7 @@ bool CChainState::LoadChainTip(const CChainParams& chainparams)
     85 
     86     if (tip && tip->GetBlockHash() == coins_cache.GetBestBlock()) return true;
     87 
     88-    if (coins_cache.GetBestBlock().IsNull() && BlockIndex().size() == 1) {
     89+    if (coins_cache.GetBestBlock().IsNull() && m_blockman.BlockIndex().size() == 1) {
     90         // In case we just added the genesis block, connect it now, so
     91         // that we always have a tip when we return.
     92         LogPrintf("%s: Connecting genesis block...\n", __func__);
     93@@ -4139,16 +4134,16 @@ bool CChainState::ReplayBlocks(const CChainParams& params)
     94     const CBlockIndex* pindexNew;            // New tip during the interrupted flush.
     95     const CBlockIndex* pindexFork = nullptr; // Latest block common to both the old and the new tip.
     96 
     97-    if (BlockIndex().count(hashHeads[0]) == 0) {
     98+    if (m_blockman.BlockIndex().count(hashHeads[0]) == 0) {
     99         return error("ReplayBlocks(): reorganization to unknown block requested");
    100     }
    101-    pindexNew = BlockIndex()[hashHeads[0]];
    102+    pindexNew = m_blockman.BlockIndex()[hashHeads[0]];
    103 
    104     if (!hashHeads[1].IsNull()) { // The old tip is allowed to be 0, indicating it's the first flush.
    105-        if (BlockIndex().count(hashHeads[1]) == 0) {
    106+        if (m_blockman.BlockIndex().count(hashHeads[1]) == 0) {
    107             return error("ReplayBlocks(): reorganization from unknown block requested");
    108         }
    109-        pindexOld = BlockIndex()[hashHeads[1]];
    110+        pindexOld = m_blockman.BlockIndex()[hashHeads[1]];
    111         pindexFork = LastCommonAncestor(pindexOld, pindexNew);
    112         assert(pindexFork != nullptr);
    113     }
    114@@ -4235,7 +4230,7 @@ bool CChainState::RewindBlockIndex(const CChainParams& params)
    115     // blocks will be dealt with below (releasing cs_main in between).
    116     {
    117         LOCK(cs_main);
    118-        for (const auto& entry : BlockIndex()) {
    119+        for (const auto& entry : m_blockman.BlockIndex()) {
    120             if (IsWitnessEnabled(entry.second->pprev, params.GetConsensus()) && !(entry.second->nStatus & BLOCK_OPT_WITNESS) && !m_chain.Contains(entry.second)) {
    121                 EraseBlockData(entry.second);
    122             }
    123@@ -4373,7 +4368,7 @@ bool LoadBlockIndex(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs
    124     if (!fReindex) {
    125         bool ret = LoadBlockIndexDB(chainparams);
    126         if (!ret) return false;
    127-        needs_init = g_chainman.m_blockman.m_block_index.empty();
    128+        needs_init = g_chainman.m_blockman.BlockIndex().empty();
    129     }
    130 
    131     if (needs_init) {
    132@@ -4553,17 +4548,17 @@ void CChainState::CheckBlockIndex(const Consensus::Params& consensusParams)
    133     // so we have the genesis block in BlockIndex() but no active chain. (A few of the tests when
    134     // iterating the block tree require that m_chain has been initialized.)
    135     if (m_chain.Height() < 0) {
    136-        assert(BlockIndex().size() <= 1);
    137+        assert(m_blockman.BlockIndex().size() <= 1);
    138         return;
    139     }
    140 
    141     // Build forward-pointing map of the entire block tree.
    142     std::multimap<CBlockIndex*,CBlockIndex*> forward;
    143-    for (const std::pair<const uint256, CBlockIndex*>& entry : BlockIndex()) {
    144+    for (const std::pair<const uint256, CBlockIndex*>& entry : m_blockman.BlockIndex()) {
    145         forward.insert(std::make_pair(entry.second->pprev, entry.second));
    146     }
    147 
    148-    assert(forward.size() == BlockIndex().size());
    149+    assert(forward.size() == m_blockman.BlockIndex().size());
    150 
    151     std::pair<std::multimap<CBlockIndex*,CBlockIndex*>::iterator,std::multimap<CBlockIndex*,CBlockIndex*>::iterator> rangeGenesis = forward.equal_range(nullptr);
    152     CBlockIndex *pindex = rangeGenesis.first->second;
    153diff --git a/src/validation.h b/src/validation.h
    154index 84605e957c..20a90cda0b 100644
    155--- a/src/validation.h
    156+++ b/src/validation.h
    157@@ -465,8 +465,12 @@ struct CBlockIndexWorkComparator
    158  * candidate tips is not maintained here.
    159  */
    160 class BlockManager {
    161-public:
    162     BlockMap m_block_index GUARDED_BY(cs_main);
    163+public:
    164+    BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
    165+    {
    166+        return m_block_index;
    167+    }
    168 
    169     /** In order to efficiently track invalidity of headers, we keep the set of
    170       * blocks which we tried to connect and found to be invalid here (ie which
    171@@ -488,7 +492,6 @@ public:
    172       */
    173     std::set<CBlockIndex*> m_failed_blocks;
    174 
    175-public:
    176     /*
    177      * All pairs A->B, where A (or one of its ancestors) misses transactions, but B has transactions.
    178      * Pruned nodes may have entries where B is missing data.
    179@@ -775,9 +778,6 @@ public:
    180     /** Check whether we are doing an initial block download (synchronizing from disk or network) */
    181     bool IsInitialBlockDownload() const;
    182 
    183-    //! [@returns](/bitcoin-bitcoin/contributor/returns/) the map of blocks that this chainstate is aware of.
    184-    BlockMap& BlockIndex();
    185-
    186     /** Update the chain tip based on database information. */
    187     bool LoadChainTip(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    188 
    189@@ -1040,7 +1040,7 @@ public:
    190 
    191     BlockMap& BlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
    192     {
    193-        return m_blockman.m_block_index;
    194+        return m_blockman.BlockIndex();
    195     }
    196 
    197     bool IsSnapshotActive() const
    198diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
    199index 91618525fd..afb619f13f 100644
    200--- a/src/wallet/test/wallet_tests.cpp
    201+++ b/src/wallet/test/wallet_tests.cpp
    202@@ -272,7 +272,7 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64
    203     if (blockTime > 0) {
    204         LockAnnotation lock(::cs_main);
    205         auto locked_chain = wallet.chain().lock();
    206-        auto inserted = ::ChainstateActive().BlockIndex().emplace(GetRandHash(), new CBlockIndex);
    207+        auto inserted = g_chainman.BlockIndex().emplace(GetRandHash(), new CBlockIndex);
    208         assert(inserted.second);
    209         const uint256& hash = inserted.first->first;
    210         block = inserted.first->second;
    

    jamesob commented at 8:33 pm on May 8, 2019:
    Yeah I like your suggestion, thanks! Will work it into the commits.
  126. sidhujag referenced this in commit dced7ccc66 on May 7, 2019
  127. DrahtBot added the label Needs rebase on May 8, 2019
  128. in src/validation.cpp:1479 in d49425d82f outdated
    1040         return true;
    1041-    if (::ChainActive().Tip() == nullptr)
    1042+    if (m_chain.Tip() == nullptr)
    1043         return true;
    1044-    if (::ChainActive().Tip()->nChainWork < nMinimumChainWork)
    1045+    if (m_chain.Tip()->nChainWork < nMinimumChainWork)
    


    Sjors commented at 4:49 pm on May 8, 2019:
    This means the catchup process never leaves IBD once nMinimumChainWork > work(assume_utxo block). Not sure if that matters.
  129. Sjors commented at 7:05 pm on May 8, 2019: member

    Did another test (on Ubuntu) and was able to create a snapshot and load it in a fresh datadir.

    When I tried to make a snapshot while still in IBD, the chain reorged down to the specified height and then immedidately shot up again to the tip, before making a (presumable wrong) snapshot. Easiest solution is for the demo script to check if initialblockdownload is false.

    Perhaps related, but I managed to corrupt the node that I took the snapshot from:

    02019-05-08T17:35:39Z Checking all blk files are present...
    12019-05-08T17:35:40Z Initializing chainstate Chainstate [ibd] @ height -1 (null)
    2bitcoind: validation.cpp:2448: void CChainState::PruneBlockIndexCandidates(): Assertion `!setBlockIndexCandidates.empty()' failed.
    

    I recovered with -reindex and then made the snapshot more carefully.

    When I restart the node during the snapshot sync, I notice that monitorsnapshot returns the wrong values for verification_progress of snapshot (e.g. 0.142) which doesn’t go up much until you restart again (initialblockdownload: false). I restarted a few times, and then it suddenly showed 0.9999 again.

    The new logging every 2000 blocks feature doesn’t work after restart.

    The snapshot successfully validated, but when I try to shutdown gracefully it doesn’t get beyond the “dumped mempool” message. Probably a thread locking issue; no crazy CPU or memory usage. I had to use kill -9, but fortunately it still worked at the next start.

    I haven’t tried pruning yet.

  130. laanwj referenced this in commit 5d37c1bde0 on Jun 5, 2019
  131. sidhujag referenced this in commit 252b7cf94b on Jun 6, 2019
  132. jamesob force-pushed on Jun 7, 2019
  133. jamesob force-pushed on Jun 7, 2019
  134. DrahtBot removed the label Needs rebase on Jun 7, 2019
  135. maflcko added the label Needs rebase on Jun 8, 2019
  136. DrahtBot removed the label Needs rebase on Jun 8, 2019
  137. jamesob force-pushed on Jun 11, 2019
  138. laanwj referenced this in commit 8f604361eb on Jul 16, 2019
  139. jamesob force-pushed on Jul 18, 2019
  140. jamesob force-pushed on Jul 18, 2019
  141. jamesob force-pushed on Jul 18, 2019
  142. fanquake referenced this in commit 848f245d04 on Jul 23, 2019
  143. jamesob force-pushed on Jul 23, 2019
  144. sidhujag referenced this in commit 17e7b271dd on Jul 29, 2019
  145. maflcko referenced this in commit 85883a9f8e on Aug 15, 2019
  146. jamesob force-pushed on Aug 19, 2019
  147. maflcko commented at 10:32 pm on August 23, 2019: member

    Can you split out d94356e086a2240773a42649ce6ef785b3a3d4b5 into a separate pull request? I need this for some of my fuzzing projects.

    See #16703

  148. maflcko referenced this in commit a7be1cc92b on Aug 27, 2019
  149. sidhujag referenced this in commit 717747348e on Aug 27, 2019
  150. jamesob force-pushed on Aug 27, 2019
  151. Sjors commented at 12:30 pm on August 28, 2019: member

    On macOS 10.14.6 I’m seeing a warning:

    0validation.cpp:2222:64: warning: lambda capture 'func_name' is not required to be captured for this use [-Wunused-lambda-capture]
    1    auto log_progress = [pindexNew, &coins_view, &chainParams, func_name](
    2                                                             ~~^~~~~~~~~
    3validation.cpp:4184:50: warning: comparison of integers of different signs: 'int64_t' (aka 'long long') and 'size_t' (aka 'unsigned long') [-Wsign-compare]
    4        if (nCheckLevel >= 3 && curr_coins_usage <= chainstate.m_coinstip_cache_size_bytes) {
    5                                ~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    62 warnings generated.
    

    Can you make the snapshot a bit more recent?

    I tried loading the snapshot I created back in May and noticed 2019-08-28T12:11:37Z [snapshot] waiting to see blockheader 000000000000001629aa3af57b421626b17c233ad0cd7197dfcc77389fe7b85e in headers chain before snapshot activation burried in the log. Oops, that’s a testnet snapshot :-) But this causes the RPC call to hang and makes it difficult to exit bitcoind, so we should probably detect this.

    Update: I made a fresh snapshot of a more recent block. When trying to import it the debug log said 2019-08-28T17:34:46Z [snapshot] assumeutxo value in snapshot metadata not valid for height 592000 - refusing to load snapshot, but the RPC returned a cryptic error code: -32603. error message: Unable to load UTXO snapshot ....

    flushing snapshot chainstate to disk takes 10+ minutes. Can this step be skipped if -dbcache is large enough?

    Is this ignoring my -dbcache setting?

    02019-08-28T17:55:49Z [Chainstate [snapshot] @ height 592003 (0000000000000000000919cc22af382ead360cd7a27572e19a2b6ac42baa6745)] resized coinsdb cache to 0.4 MiB
    12019-08-28T17:55:49Z [Chainstate [snapshot] @ height 592003 (0000000000000000000919cc22af382ead360cd7a27572e19a2b6ac42baa6745)] resized coinstip cache to 499.5 MiB
    22019-08-28T17:55:49Z Cache size (8699974960) exceeds total space (1523763712)
    
  152. Sjors commented at 1:20 pm on August 29, 2019: member

    I made a torrent tracker for these snapshots, as well as limited capacity seeding:

    • mainnet: magnet:?xt=urn:btih:556fb8dcc50059a62b0694f576a14e249156ab99&dn=utxo%5Fsnapshot%5F570000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969
    • testnet: magnet:?xt=urn:btih:8780d3e8e336986a7fede11bea3e1209e2b25e41&dn=utxo%5Fsnapshot%5Ftestnet%5F1512062.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969
  153. jamesob force-pushed on Sep 12, 2019
  154. jamesob force-pushed on Sep 16, 2019
  155. jamesob force-pushed on Sep 16, 2019
  156. jamesob commented at 6:05 pm on September 17, 2019: member

    At the moment, the tip of this branch is broken - there’s a lock inversion between the new g_chainman.m_cs_chainstates lock and mempool.cs. It’s proven tricky to untangle because ChainstateActive() (which requires g_chainman.m_cs_chainstates) is called so pervasively.

    I tried swapping out m_cs_chainstates for cs_main since they’re pretty much always overlapping anyways, but that’s a non-starter because you can’t call g_chainman.GetAll() on top of chainstate->ActivateBestChain() because ABC asserts that cs_main isn’t held (to drain the validation queue).

    I’m trying to work out the lowest-impact fix.

    Notes

    02019-08-20T17:18:51Z POTENTIAL DEADLOCK DETECTED
    12019-08-20T17:18:51Z Previous lock order was:
    22019-08-20T17:18:51Z  m_cs_chainstate validation.cpp:2675 (in thread loadblk)
    32019-08-20T17:18:51Z  cs_main validation.cpp:2693 (in thread loadblk)
    42019-08-20T17:18:51Z  (1) ::mempool.cs validation.cpp:2693 (in thread loadblk)
    52019-08-20T17:18:51Z  (2) g_chainman.m_cs_chainstates validation.cpp:86 (in thread loadblk)
    

    which is

    ActivateBestChain: LOCK(mempool.cs) -> ConnectTip: LOCK(g_chainman.m_cs_chainstates) via ChainstateActive()

    02019-08-20T17:18:51Z Current lock order is:
    12019-08-20T17:18:51Z  cs_main init.cpp:1484 (in thread init)
    22019-08-20T17:18:51Z  (2) m_cs_chainstates validation.cpp:5507 (in thread init)
    32019-08-20T17:18:51Z  cs_main validation.cpp:2046 (in thread init)
    42019-08-20T17:18:51Z  (1) cs txmempool.cpp:907 (in thread init)
    

    which is

    ChainstateManager.MaybeRebalanceCaches(): LOCK(m_cs_chainstates) -> FlushStateToDisk -> mempool.DynamicMemoryUsage(): LOCK(mempool.cs)

  157. maflcko referenced this in commit 7d4bc60f1f on Sep 19, 2019
  158. jamesob force-pushed on Sep 23, 2019
  159. sidhujag referenced this in commit d8a09acbc9 on Sep 23, 2019
  160. ryanofsky commented at 3:29 pm on September 24, 2019: contributor

    It’s proven tricky to untangle because ChainstateActive() (which requires g_chainman.m_cs_chainstates) is called so pervasively.

    Can’t remember what we talked about last time this came up, but maybe m_cs_chainstates should be a shared mutex, that many threads can call lock_shared on simultaneously, and not block unless the chain is being swapped.

    That way m_cs_chainstates could be locked at a really broad scope, before cs_main and mempool.cs, but it wouldn’t hurt performance because it would be locked non-exclusively.

  161. jamesob force-pushed on Sep 27, 2019
  162. laanwj added the label Feature on Sep 30, 2019
  163. laanwj referenced this in commit a37f4c220a on Oct 30, 2019
  164. laanwj referenced this in commit b05b28183c on Nov 5, 2019
  165. Sjors commented at 10:38 am on November 6, 2019: member

    I’m hosting some recent snapshots as a torrent (and tracker):

    • 570,000 mainnet magnet:?xt=urn:btih:556fb8dcc50059a62b0694f576a14e249156ab99&dn=utxo%5Fsnapshot%5F570000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969 (made with older version, probably incompatible)
    • 600,000 mainnet: magnet:?xt=urn:btih:e3c0b504c8e60c52653706079dbfecc5bcad5e02&dn=utxo%5Fmainnet%5F600000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969
    • 1,600,000 testnet: magnet:?xt=urn:btih:701acffe49e83bce213ff337e52022c11368cdd0&dn=utxo%5Ftestnet%5F1600000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969
  166. sidhujag referenced this in commit 9df460f09f on Nov 7, 2019
  167. jamesob force-pushed on Nov 8, 2019
  168. jamesob force-pushed on Nov 13, 2019
  169. jamesob force-pushed on Nov 15, 2019
  170. jamesob force-pushed on Nov 18, 2019
  171. jamesob commented at 9:17 pm on November 18, 2019: member

    A few quick updates on this branch.

    New snapshots

    I’ve added an allowed assumeutxo hash at height 600,000 (which you can check with ./contrib/devtools/utxo_snapshot.sh - ./src/bitcoin-cli -datadir=[...]) and have uploaded snapshot files (Google Drive, Google Cloud) for testers’ convenience. We obviously don’t necessarily need to preserve this value for merge, but I figure it’s useful for testing and not unrealistic.

    You can test this branch easily by running a script like

     0#!/bin/bash
     1
     2set -e
     3if [ ! -f ./src/bitcoind ]; then
     4  echo "Must be in built bitcoin directory."
     5  exit 1
     6fi
     7SNAPSHOT_LOC=/tmp/utxo.600k.dat
     8DATADIR=/tmp/bitcoin-assumeutxo-test
     9mkdir -p $DATADIR
    10
    11[ -f ${SNAPSHOT_LOC} ] || curl -L https://storage.googleapis.com/assumeutxo/utxo.600k.dat > $SNAPSHOT_LOC
    12/usr/bin/time -v ./src/bitcoind -datadir=$DATADIR -dbcache=4000 -stopatheight=604401 &
    13sleep 60
    14# Might timeout.
    15/usr/bin/time -v ./src/bitcoin-cli -datadir=$DATADIR loadtxoutset $SNAPSHOT_LOC || true
    16fg %1
    

    After running this you should end up with a node that’s synced to tip within 60-90 minutes depending on your internet connection and hardware.

    Benchmarking

    I’ve compared reindexing to height 550,000 for this branch vs. its base (a la previous PRs), and beyond some outlier runs on both sides, this changeset seems to be comparable in performance to master.

     0host         tag                      time       time% maxmem  cpu%  dbcache
     1bench-ssd-2  bench/au.1               9:24:53    0.92  6717.99MB 338%  5000MB
     2bench-ssd-2  bench/au.1               9:23:14    0.92  6770.57MB 339%  5000MB
     3bench-ssd-2  bench/au.master.1        10:08:33   0.99  6654.39MB 315%  5000MB
     4bench-ssd-2  bench/au.master.1        10:13:04   1.00  6697.26MB 313%  5000MB
     5
     6bench-ssd-3  bench/au.master.1        9:26:17    0.93  6743.38MB 337%  5000MB
     7bench-ssd-3  bench/au.master.1        9:24:03    0.93  6748.67MB 338%  5000MB
     8bench-ssd-3  bench/au.1               9:22:35    0.92  6713.50MB 340%  5000MB
     9bench-ssd-3  bench/au.1               10:08:36   1.00  6712.88MB 314%  5000MB
    10
    11bench-ssd-4  bench/au.1               9:24:59    0.93  6744.43MB 338%  5000MB
    12bench-ssd-4  bench/au.1               10:06:09   1.00  6717.30MB 315%  5000MB
    13bench-ssd-4  bench/au.master.1        9:23:44    0.93  6695.61MB 339%  5000MB
    14bench-ssd-4  bench/au.master.1        9:23:56    0.93  6646.16MB 338%  5000MB
    15
    16bench-ssd-5  bench/au.master.1        9:18:57    1.00  6715.23MB 341%  5000MB
    17bench-ssd-5  bench/au.master.1        9:20:35    1.00  6644.22MB 341%  5000MB
    18bench-ssd-5  bench/au.1               9:17:27    0.99  6702.63MB 342%  5000MB
    19bench-ssd-5  bench/au.1               9:21:17    1.00  6663.15MB 340%  5000MB
    

    Note of course that as a sort of “meta”-optimization, these benchmarks are solely to ensure that we haven’t introduced any performance regressions into normal operation. The really relevant benchmark here is “time from empty datadir to usable node,” which on one of my more capable machines has gone from ~20hrs to ~1hr.

    Next steps

    If you want to help move this project forward, the following PRs are prerequisites and once they’re merged I’ll carve off the next changeset:

    Current work

    After significantly revising the structure of the snapshot file in #16899, this branch has been substantially rebased to avoid the requirement of any on-disk snapshot metadata once the snapshot has been loaded; instead we look to filesystem state (the presence of a $DATADIR/chainstate_* folder) to signal that we are currently building a snapshot chainstate. Once this chainstate has been fully back validated, we move it over to the regular chainstate/ dir on shutdown. We also used to need on-disk state to restore the “faked” nChainTx data for snapshot-based header chains – we now cache that information in the snapshot’s chainstate itself (https://github.com/bitcoin/bitcoin/pull/15606/commits/87093a94ba311c495a1982f4f47d0c4607789f8f).

    I’m currently thinking about how to handle locking semantics (as you might be able to tell from the last commit, which needs to be squashed into the rest of the history at some point) which I can detail further if anyone’s interested.

  172. jamesob force-pushed on Nov 20, 2019
  173. jamesob force-pushed on Nov 21, 2019
  174. jamesob force-pushed on Dec 3, 2019
  175. jamesob force-pushed on Dec 12, 2019
  176. laanwj referenced this in commit 2ed74a43a0 on Jan 13, 2020
  177. jamesob force-pushed on Jan 13, 2020
  178. sidhujag referenced this in commit ab2fb60cfa on Jan 14, 2020
  179. deadalnix referenced this in commit f99b18b3f6 on Feb 29, 2020
  180. deadalnix referenced this in commit 8b0ad8e956 on Mar 2, 2020
  181. maflcko referenced this in commit 10358a381a on Apr 10, 2020
  182. jamesob force-pushed on Apr 13, 2020
  183. sidhujag referenced this in commit d35bad573a on Apr 13, 2020
  184. maflcko commented at 3:06 pm on April 20, 2020: member

    Commits that can be split out and submitted for review in parallel:

    • f416c16b5728c91b071e18ea860a59c0401dcb5d
    • ba1f760cdad07331b403966a0d3ab8b3f9898799 (partially, at least)
    • e75fbc3087c1ad5039c946a0ac3107d6d2c84b94 (after #18698 ?)

    commit 9b665e999bc82b98ff317907370db6f65efb2b43 can be dropped, no? It is already properly marked:

    0$ git grep 'bool LoadBlockIndex('
    1src/validation.cpp:bool LoadBlockIndex(const CChainParams& chainparams)
    2src/validation.h:bool LoadBlockIndex(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    3src/validation.h:    bool LoadBlockIndex(
    
  185. jamesob force-pushed on May 6, 2020
  186. ftrader referenced this in commit 358200ce3a on May 19, 2020
  187. ftrader referenced this in commit 8e404431cc on May 19, 2020
  188. maflcko commented at 12:32 pm on May 23, 2020: member
    Why is GitHub so horribly useless? I can’t even see the last comment I posted here two weeks ago without spending minutes unwrapping the discussion
  189. maflcko referenced this in commit 2f71a1ea35 on Jul 29, 2020
  190. monstrobishi referenced this in commit c2582816b2 on Jul 30, 2020
  191. sidhujag referenced this in commit 191c48d32d on Jul 31, 2020
  192. jamesob force-pushed on Aug 1, 2020
  193. jamesob commented at 7:51 pm on August 1, 2020: member

    Quick update. I’ve rebased this PR in such a way that it does not rely on changes in #17487 to avoid being blocked on that going through. If that does merge, it’ll be easy to update this branch to make use of the flush-without-erase behavior.

    This needs testing (which I plan to do soon) as it’s been a while since I’ve gone through the whole battery of manual system tests (i.e. actually loading snapshots and syncing). I need to focus on writing a few more functional and unit tests that will obviate most of that manual work. Some intermediate commits here almost certainly have compilation errors.

    This project is getting to the point where it’s less straightforward to find discrete bits of this draft to carve off for individual merge. Among the next changes are the snapshot activation procedure, the related RPC command for loading a snapshot, and all the little correspondent changes that have to happen in modules like net_processing to have snapshot chainstates actually function. I guess it makes sense to PR the snapshot activation changes (https://github.com/bitcoin/bitcoin/pull/15606/commits/eae9c625b10a7b5e6495ebef8698a98873f4933a) individually, but after that it may be somewhat less straightforward to carve off commits.

    I’m wondering how averse people would be to treating much of the remainder of this draft as a single PR, and subsequently finishing off the first phase of assumeutxo in two or so more PRs?

  194. maflcko commented at 5:45 am on August 2, 2020: member
    I still thinks this can and should be split up further, since that makes it easier to find reviewers for the part of the code that is modified. For example, the net_processing additions might be reviewed by a different set of people than the validation changes like 8ce5eb04d5ec21843e6a3919d0beee9f6e6589fe
  195. ShengguangXiao referenced this in commit 86ea1b6fe6 on Aug 28, 2020
  196. sidhujag referenced this in commit f0b0681fa2 on Nov 10, 2020
  197. sidhujag referenced this in commit 8e75779f0e on Nov 10, 2020
  198. UdjinM6 referenced this in commit 9ab9422d7b on Nov 17, 2020
  199. UdjinM6 referenced this in commit 36d275396f on Dec 1, 2020
  200. PastaPastaPasta referenced this in commit b559a8f904 on Dec 15, 2020
  201. laanwj referenced this in commit 92fee79dab on Feb 16, 2021
  202. Sjors commented at 3:29 pm on February 17, 2021: member
    Rebase time now that #19806 landed?
  203. jamesob commented at 3:35 pm on February 17, 2021: member

    Rebase time now that #19806 landed?

    Yup, actively working on it.

  204. jamesob force-pushed on Feb 28, 2021
  205. jamesob force-pushed on Mar 24, 2021
  206. ryanofsky commented at 1:32 am on April 1, 2021: contributor

    Approach ACK / light review ACK 285d5dd3e409c1f14f50c46f18b3b1338f7ab9f6

    Looking at the remaining commits it seems like the assumeutxo implementation is actually 75% merged already, and remaining changes are mostly tweaking code that hardcodes or assumes ActiveChainstate to instead handle multiple chainstates.

    Current approach of splitting this large PR into medium sized PRs to be reviewed and merged sequentially (starting with #21526) seems reasonable and is probably ideal. I could also imagine a different approach thats splits out the tweaky commits as small obvious 1-commit PRs that could be reviewed in parallel, but this might just be more work and not solve anything.

    In an ideal world it would be nice if individual changes here could be accompanied by unit tests confirming the new behaviors, but understandable that this isn’t happening since validation code is not very modular.

  207. maflcko referenced this in commit c6d6bc8abb on Apr 27, 2021
  208. Sjors commented at 1:46 pm on May 21, 2021: member
    Would be nice to see a rebase of this, as it’s a bit easier to understand the prerequisite PR’s in their full context.
  209. maflcko commented at 8:37 am on May 25, 2021: member
    Agree with @Sjors . Currently it is not possible to test the “great picture” and only unit test or code review the split out patches.
  210. jamesob commented at 4:23 pm on May 25, 2021: member
    Thanks for the pings, I’ll start this tomorrow.
  211. jamesob force-pushed on May 27, 2021
  212. jamesob commented at 8:56 pm on May 27, 2021: member

    utxo-dumpload.61 -> utxo-dumpload.62 (range-diff)

    Large rebase encompassing

    • many net_processing refactors by @jnewbery @rebroad et al. (#21713)
    • @dongcarl’s chainstate deglobalization (#20158)
    • pruning updates for the blockfilter index (#15946)
    • and probably others I’m forgetting.

    In the next few days I’ll be manually testing this, which will require re-adding an actual assumeutxo value in chainparams after generating a recent snapshot. While or after that’s happening, I’ll see about generating a snapshot for use by the functional tests (and then actually writing them).

  213. jamesob force-pushed on May 27, 2021
  214. Sjors commented at 6:42 pm on May 31, 2021: member

    Torrent for my snapshot at height 685,000: magnet:?xt=urn:btih:bbc943dd7a97bff4bab7a039414e4dae5fbf5a3c&dn=utxo%5Fmainnet%5F685000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969

    To make this snapshot, I rolled back to the correct height: bitcoin-cli invalidateblock 00000000000000000002091619d7ae1596349afb10fe8cb1833a09945db1180d and then called dumptxoutset (same procedure as contrib/devtools/utxo_snapshot.sh).

    It’s probably broken given that coins_written is 0

    0{
    1  "coins_written": 0,
    2  "base_hash": "000000000000000000043a68d9e09f48eeb485ac28bde0abb6e0b6cb6e726a21",
    3  "base_height": 685000,
    4  "path": "...",
    5  "assumeutxo": "771ad185615493278a5d62b4fa865ca47cbfc54dbeb3a9157535bc0d856369e4"
    6}
    

    When attempting to load for a fresh IBD, by adding the snapshot to chainparams.cpp:

    0        m_assumeutxo_data = MapAssumeutxo{
    1            {
    2                685000,
    3                {AssumeutxoHash{uint256S("0x771ad185615493278a5d62b4fa865ca47cbfc54dbeb3a9157535bc0d856369e4")}, 685000},
    4            },
    5        };
    

    The load loadtxoutset call fails with [snapshot] bad snapshot - coins left over after deserializing 0 coins

  215. jamesob commented at 5:18 pm on June 8, 2021: member

    Thanks for testing @Sjors. I couldn’t reproduce your bug, but during manual testing I did find a number of issues based upon changes that have happened since last rebase. Subsequently I’ve pushed a bunch of new commits, some of which need to be fixed up. I’ve also done a manual test of the assumeutxo workflow and everything seems to be working properly again.

    I also found an issue, possibly new but possibly not, related to restarting bitcoind while using a snapshot that has not yet been fully validated. Basically, faked nTx values were not persisting and so LoadBlockIndex() was failing due to not constructing setBlockIndexCandidates as expected.

    In response to the this as well as the recent changes in RewindBlockIndex, I’ve decided to make a change consistent with a previous recommendation from @ryanofsky: instead of mutating “fake” data into the block index for assumed-valid blocks, I’m instead explicitly accounting for the possibility of snapshot use at usage sites (e.g. LoadBlockIndex(), NeedsRedownload()). I think this is easier to reason about instead of relying on some implicit data faking.

    Manual testing

    I’ve generated a snapshot at 685000 using the utxo_snapshot.sh contrib script.

     0src/bitcoin (? utxo-dumpload-compressed 4dfd3a2) % /usr/bin/time ./contrib/devtools/utxo_snapshot.sh 685000 utxo-685.dat ./src/bitcoin-cli
     1Rewinding chain back to height 685000 (by invalidating 00000000000000000002091619d7ae1596349afb10fe8cb1833a09945db1180d); this may take a while
     2Generating UTXO snapshot...
     3{
     4  "coins_written": 75696073,
     5  "base_hash": "000000000000000000043a68d9e09f48eeb485ac28bde0abb6e0b6cb6e726a21",
     6  "base_height": 685000,
     7  "path": "/home/james/.bitcoin/utxo-685.dat",
     8  "assumeutxo": "a85dd26a5ca449d76bc7cb6103960a2894475f11acef57efb77d833ca84d0ed3",
     9  "nchaintx": 644907744
    10}
    11Restoring chain to original height; this may take a while
    120.01user 0.00system 14:13.84elapsed 0%CPU (0avgtext+0avgdata 4712maxresident)k
    132688inputs+0outputs (22major+1584minor)pagefaults 0swaps
    

    I then create a new datadir (~/.bitcoin/utxo-test), start up, and immediately load the snapshot:

    0src/bitcoin (? utxo-dumpload-compressed 4dfd3a2) % /usr/bin/time ./src/bitcoin-cli -datadir=/home/james/.bitcoin/utxo-test loadtxoutset ~/.bitcoin/utxo-685.dat
    1{
    2  "coins_loaded": 75696073,
    3  "tip_hash": "000000000000000000043a68d9e09f48eeb485ac28bde0abb6e0b6cb6e726a21",
    4  "base_height": 685000,
    5  "path": "/home/james/.bitcoin/utxo-685.dat"
    6}
    70.00user 0.00system 12:49.51elapsed 0%CPU (0avgtext+0avgdata 4348maxresident)k
    83536inputs+0outputs (18major+233minor)pagefaults 0swaps
    

    After load, I verified that the snapshot chain successfully syncs to tip, caches rebalance accordingly, and background validation starts. I also verified that the snapshot tip is maintained without issue as new blocks come in.

    Next

    Functional tests will be added to this branch. I may also add a contrib script that makes “demoing” the assumeutxo workflow more straightforward.

  216. jamesob force-pushed on Jun 9, 2021
  217. Sjors commented at 5:27 pm on June 10, 2021: member

    This seems to work better:

    0{
    1  "coins_written": 74785038,
    2  "base_hash": "00000000000000000004331a2b2230ee390f3cd8969c4b533e616a6805ccf918",
    3  "base_height": 687000,
    4  "path": "/Users/sjors/Library/Application Support/Bitcoin/utxo_mainnet_687000.dat",
    5  "assumeutxo": "c6d8016d392df4a9b57c96a432347df3971b78da5b095bd2eafe71da84206892",
    6  "nchaintx": 648168241
    7}
    

    Torrent: magnet:?xt=urn:btih:65361833886f742cc519003f8f85ebfe26979a36&dn=utxo%5Fmainnet%5F687000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969

    To use the snapshot, edit chainparams.cpp:

    0        m_assumeutxo_data = MapAssumeutxo{
    1            {
    2                687000,
    3                {AssumeutxoHash{uint256S("0xc6d8016d392df4a9b57c96a432347df3971b78da5b095bd2eafe71da84206892")}, 648168241},
    4            },
    5        };
    

    With a fresh datadir I’m able to load this snapshot (-rpcclienttimeout=0 is handy) and sync to the tip. We’ll see if background sync succeeds too…

    When loading fails, e.g. because it’s the wrong height, the error returned by the RPC call could be more useful.

  218. jamesob force-pushed on Jun 14, 2021
  219. jamesob force-pushed on Jun 14, 2021
  220. jamesob force-pushed on Jun 14, 2021
  221. jamesob force-pushed on Jun 15, 2021
  222. jamesob commented at 4:15 pm on June 16, 2021: member

    On the road to writing actual functional tests (which may be somewhat of a challenge), I’ve added a demo/manual testing script (https://github.com/bitcoin/bitcoin/pull/15606/commits/322a9e9fc1d0edc3297a92618edc912c50bc0fd8) that should allow anyone to get acquainted with the assumeutxo feature.

    Using it should be easy; clone this branch, make, and then run ./contrib/devtools/test_utxo_snapshots.sh and follow the instructions. The script itself involves creating a snapshot, modifying the chainparams, and then watching the snapshot-based sync process followed by background validation. It’s also a happy-path test of the changes here. I hope others find it useful, even if it just ends up being an ephemeral thing for testing that is never merged.

  223. jamesob force-pushed on Jun 17, 2021
  224. jamesob commented at 9:30 pm on June 17, 2021: member
    I’ve done a cleanup rebase as well as pushing a basic functional test (https://github.com/bitcoin/bitcoin/pull/15606/commits/aa65b21d79d9cc0d79925879bc62e1c240a1cf59) that can probably use some embellishment.
  225. in src/validation.h:817 in 35ef764b7c outdated
    813@@ -814,6 +814,7 @@ class CChainState
    814     bool LoadBlockIndexDB(const CChainParams& chainparams) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
    815 
    816     friend ChainstateManager;
    817+    friend bool LoadGenesisBlock(const CChainParams&);
    


    ryanofsky commented at 5:31 pm on June 21, 2021:

    In commit “validation: have LoadGenesisBlock work on all chainstates” (35ef764b7c8a7ebf0760e0eff52eda95ebac1136)

    Probably this should be deleted, it’s just declaring that a function which doesn’t exist would be a friend.

  226. in src/rpc/blockchain.cpp:2633 in cd1986f7ab outdated
    2620@@ -2620,7 +2621,8 @@ UniValue CreateUTXOSnapshot(NodeContext& node, CChainState& chainstate, CAutoFil
    2621     result.pushKV("coins_written", stats.coins_count);
    2622     result.pushKV("base_hash", tip->GetBlockHash().ToString());
    2623     result.pushKV("base_height", tip->nHeight);
    2624-
    2625+    result.pushKV("path", path.string());
    


    ryanofsky commented at 5:38 pm on June 21, 2021:

    In commit “dumptxoutset: add assumeutxo key to output” (cd1986f7abe6d6c7ce0918ac8cd62864205e373e)

    Path could be mentioned in commit description


    jamesob commented at 6:36 pm on June 22, 2021:
    Fixed, thanks.
  227. in src/node/interfaces.cpp:463 in 88a90b394c outdated
    456@@ -457,6 +457,11 @@ class ChainImpl : public Chain
    457         LOCK(cs_main);
    458         return CheckFinalTx(chainman().ActiveChain().Tip(), tx);
    459     }
    460+    int getLowestBlockDataHeight() override
    461+    {
    462+        LOCK(cs_main);
    463+        return Assert(m_node.chainman)->GetSnapshotNChainTx().value_or(0);
    


    ryanofsky commented at 5:46 pm on June 21, 2021:

    In commit “wallet: avoid rescans if under the snapshot” (88a90b394c8936c695717927954b2b4299ff899b)

    This seems strange. Why would this be returning number of transactions instead of the height? This seems like it would cause the rescan_height < chain.getLowestBlockDataHeight() below to be true in cases when it should be false and ignore the wallet sync error.


    jamesob commented at 2:06 pm on June 22, 2021:
    Absolutely, this is just totally wrong. Probably used the wrong method in the process of rebasing the Assert(...)-> change in.

    jamesob commented at 3:01 pm on June 22, 2021:
    And probably indicates I should write a test for this.
  228. jamesob force-pushed on Jun 21, 2021
  229. in src/validation.cpp:3680 in 5505d17e21 outdated
    3519@@ -3512,8 +3520,11 @@ bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, Block
    3520 
    3521     // Header is valid/has work, merkle tree and segwit merkle tree are good...RELAY NOW
    3522     // (but if it does not build on our best tip, let the SendMessages loop relay it)
    3523-    if (!IsInitialBlockDownload() && m_chain.Tip() == pindex->pprev)
    3524+    if (!IsInitialBlockDownload() &&
    3525+            m_chain.Tip() == pindex->pprev &&
    3526+            this == &m_chainman.ActiveChainstate()) {
    3527         GetMainSignals().NewPoWValidBlock(pindex, pblock);
    


    ryanofsky commented at 6:55 pm on June 21, 2021:

    In commit “validation: only send ValidationInterface callbacks for active chain” (5505d17e21d886d85b614a13e7fc8a61d6bf4fe9)

    Maybe update validationinterface.h documentation to say no notifications will be sent about updates to the background chainstate. If a snapshot is loaded only notification about blocks after the snapshot are sent, IIUC


    jamesob commented at 6:23 pm on August 5, 2021:
    Fixed.
  230. in src/index/base.cpp:274 in 0575a333ca outdated
    261@@ -262,6 +262,14 @@ void BaseIndex::BlockConnected(const std::shared_ptr<const CBlock>& block, const
    262     }
    263 
    264     if (WriteBlock(*block, pindex)) {
    265+        // TODO assumeutxo: since blocks may be submitted to this
    


    ryanofsky commented at 7:06 pm on June 21, 2021:

    In commit “validation: add BackgroundBlockConnected and use it for indexing” (0575a333ca1d02ecca7f651a41572ebd62de224b)

    In general, it’s not clear to me how this PR affects indexing. I assume this TODO needs to be addressed to avoid problems, but it’s not really clear what problems may be or if fixing this fixes everything


    jamesob commented at 6:11 pm on June 22, 2021:
    I’ll elaborate on this in some form soon, but the gist of it is that if indexing has ordering requirements, it has to make particular use of the BackgroundBlockValidated callback (which is why I introduced it); luckily all of the indexers (as far as I’m aware) don’t have strict ordering requirements and each block can more or less be processed in isolation (aside from bestblock tracking, used to signal completion of index building).
  231. in src/wallet/wallet.cpp:2764 in a5bd8e91aa outdated
    2760@@ -2761,7 +2761,6 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
    2761         }
    2762         // Otherwise refuse to rescan if we're operating on a snapshot and the
    2763         // rescan height is at or lower than the base of the snapshot.
    2764-        //
    


    ryanofsky commented at 7:09 pm on June 21, 2021:

    In commit “validation: introduce ChainstateManager::GetChainstateForNewBlock” (a5bd8e91aaf77853f1162a21f84efbe0e9754d5f)

    Probably wallet change here is meant to be part of earlier wallet commit

  232. jamesob force-pushed on Jun 21, 2021
  233. jamesob force-pushed on Jun 21, 2021
  234. Sjors commented at 9:27 am on June 22, 2021: member

    I was able to complete an IBD with my earlier snapshot using c78a2c743b5cedd9b9948ad355268798baa01bee (and a modified chain params).

    I noticed it didn’t clean up the cainstate_blah_hash directory after it was done. But I didn’t bother checking if a restart fixed that (already wiped that data dir).

  235. jamesob force-pushed on Jun 22, 2021
  236. jamesob force-pushed on Jun 22, 2021
  237. jamesob commented at 6:47 pm on June 22, 2021: member

    Thanks for the helpful commentary @ryanofsky and thanks for the testing @Sjors.

    I’ve pushed a cleanup rebase that addresses some of your comments, slims the commit count down to 30 (from 37), and reorders the commits to be closer to the order I anticipate they’ll be merged in. I understand some explication is required on things like indexation and P2P processing, and I’ll include that in the constituent PRs.

    This branch is now close to something resembling mergeability, though of course that’s not my expectation. I’m somewhat concerned with the pace of this project (although extenuating circumstances like taproot not to mention my own (in)ability to keep up with this work over the last year provide some explanation).

    Anyway, I’m going to try a different tack here and open a bunch of smallish, independently mergeable PRs containing some of the commits here that shouldn’t change any behavior until multi-chainstate use actually becomes possible (i.e. with the merge of the loadtxoutset PR which is intentionally towards the back of the queue).

    Thanks again to everyone for their help and patience on this project. Some of the code here was written 2 years ago for the bare purpose of coming up with a proof of concept and has been dragged along through many rebases, so everyone’s participation here helping to shape up and vet this code is very much appreciated on my end.

  238. jamesob force-pushed on Jun 22, 2021
  239. Sjors commented at 12:33 pm on June 23, 2021: member

    Anyway, I’m going to try a different tack here and open a bunch of smallish, independently mergeable PRs containing some of the commits here that shouldn’t change any behavior

    With the big picture in this PR being up to date, it should be easier to review your (upcoming) smaller PR’s. Forest, trees, clear.

  240. ryanofsky commented at 2:48 pm on June 25, 2021: contributor

    Notes from going through this PR and writing down the behavior changes:

    • UpdateTip() - skip updating mempool transactions count for background chainstate
      • would be better if background chainstate mempool pointer was just null?
    • UpdateTip() - skip activation warning for background chainstate
      • would be better if background chainstate just considered always IBD? (since the warnings are already skipped for IBD)
    • CChainState::CheckBlockIndex() - skip snapshot chainstate block havedata checks
      • would be better if pindexFirstMissing, pindexFirstNeverProcessed, setBlockIndexCandidates were chainstate members instead of globals?
    • CChainState::FlushStateToDisk() - suppress notification for background chainstate (used for wallet bestblock)
    • CChainState::DisconnectTip() - suppress notification for background chainstate
      • unclear why disconnect would happen for background chainstate
    • CChainState::ConnectTip() - suppress notification for background chainstate
    • CChainState::ActivateBestChain() - suppress notifications (BlockConnected, UpdatedBlockTip, NotifyBlockTip) for background chainstate
    • CChainState::AcceptBlock() - suppress notification (NewPoWValidBlock) for background chainstate
    • would be better to suppress based on IBD?
    • ChainstateManager::ProcessNewBlock() - call CChainState::AcceptBlock() for background chainstate
    • FindNextBlocksToDownload() - replace global pindexLastCommonBlock with per-chainstate chainstate_to_last_common_block
    • PeerManagerImpl::ProcessMessage & PeerManagerImpl::MaybeSendAddr() - consider node to be in IBD state and not advertise if there is a background chain
    • AppInitMain() - call InitCoinsDB earlier, and use smaller starting cache size, and add MaybeRebalanceCaches call
    • Shutdown() - stop calling ResetCoinsViews (why?)
    • AppInitMain() - call CheckForUncleanShutdown
    • CChainState::ConnectTip() - call CompleteSnapshotValidation
    • CChainState::ActivateBestChain() - return early if m_stop_use unexpected error condition is true
    • CChainState::ActivateBestChain() - call MaybeRebalanceCaches if ibd changes true to false
    • BlockManager::FindFilesToPruneManual() - avoid pruning background chainstate blocks
    • CChainState::ConnectTip() - avoid calling m_mempool.removeForBlock and disconnectpool.removeForBlock for background chainstate
    • ChainstateManager::ProcessNewBlockHeaders() - call chainstate->CheckBlockIndex for all chains
    • BlockManager::LoadBlockIndex() - set pindex->nChainTx for first block after snapshot
    • BlockManager::LoadBlockIndex() - loop over chains to set setBlockIndexCandidates for all chains
    • CChainState::NeedsRedownload() - stop returning true for background chainstate download blocks because they don’t have witness data flag set yet. this doesn’t affect download just avoids “missing data, please reindex” error on startup
  241. ryanofsky commented at 3:51 pm on June 25, 2021: contributor

    Going through the changes in this PR, they seem reasonable, but not as straightforward as they could be. IMO, with a modest reworking, the assumeutxo logic could be better encapsulated and implemented in ChainStateManager without adding as many special cases in validation and other code.

    The change I’d suggest would be to drop all the ChainStateManager m_*_chainstate members and just have a plain list of chainstates: std::vector<std::unique_ptr<CChainState>> m_chainstates, and change each chainstate CTxMemPool& m_mempool reference member to a CTxMemPool* m_mempool pointer member which is non-null for the active chainstate, null for any other chainstates. Validation code would loop over chainstates and update the mempool when it’s non-null, skip it when it’s null, send a BackgroundBlockConnected notification if it’s null or BlockConnected if non-null, etc.

    This would also allow;

    • Dropping a lot of assumeutxo jargon. I still get “IBD chainstate” “Snapshot chainstate” “Active chainstate” “Background IBD chainstate” “Validated chainstate” confused and have to think about what they mean each time I see them in context. The current distinctions between “valid” and “validated” (valid by assumption vs validated by downloading blocks in the background) also seem unnecessarily opaque. These definitions seem unneeded if the assumeutxo implementation is confined to ChainStateManager and the assumeutxo RPC methods, and the rest of the code can just loop over chainstates treating them the same and just skipping some work if m_mempool is null.

    • More straightforward approach to pruning. Instead of having to write special case pruning logic, this could be integrated with #21726, and each chainstate could just add to a list of prune blockers (ranges of blocks not allowed to be pruned) and pruning code would not need to be aware of assumeutxo states.

    • Keeping future code maintainable and having confidence in completeness of the change. Fact that assumeutxo implementation is not well encapsulated now makes it hard to be confident that the long list of assumeutxo special cases in #15606 (comment) is actually complete, and that special cases added now won’t break in the future if there are assumeutxo changes later

  242. jamesob commented at 6:33 pm on June 28, 2021: member

    Russ, thanks for the thorough and thoughtful look here. Really appreciate your design guidance.

    Of course I have the predictable grumbles about the rework necessary and a slight dread at the prospect of introducing more prerequisite refactoring (given this project has been ongoing for two years already), but on design questions like this I’m inclined to side with you given (i) this code began as a minimum-viable demo prototype, and (ii) I’ve been staring at it for too damn long to be objective.

    Correct me if I’m wrong, but one of the motivating factors behind your suggestion here is the notion that we may one day be facilitating n background chainstates instead of just 1, at which point some rework would be required to fix the abstractions for that case. Indeed the utreexo project that @kcalvinalvin @adiabat et al. have been working on may introduce such a usage.

    Just a few potshot responses before really gauging the level of effort required to make the changes you’re talking about:

    HasMempool vs IsActiveChain

    One pushback I might have is that a lot of the “inline” logic that’s scattered throughout parts of validation and net_processing won’t actually by simplified by relatively surface-level transformations of == ActiveChainstate() -> HasMempool(). In some cases switching behavior on mempool presence might be even more confusing (e.g. https://github.com/bitcoin/bitcoin/pull/15606/commits/b51d6d7a30bc6c9bd0a63549254156ae3aeb6d4a) since the mempool is basically unrelated to things like validationinterface callbacks. But maybe making that change for cases where the mempool is relevant might make sense and be pretty easy (https://github.com/bitcoin/bitcoin/pull/15606/commits/70395ccedafd54fda2bc2e634e631935f5f1cec3).

    Fundamentally a lot of the validation/net logic just needs to know whether it’s dealing with the active chainstate or not; I don’t see a way around that short of moving a maybe-inappropriate amount of domain-specific code into the ChainstateManager.

    supporting n chainstates

    If our goal is to facilitate n background chainstates instead of just 1 (although maybe your goal is purely to make the code easier to follow?), my inclination would be to say that we should wait until we’re actually looking to implement n chainstates before spending a bunch of time on design changes.

    On list-of-chainstates vs. m_ibd_chainstate and m_snapshot_chainstate, these are private members of chainman and so swapping them out for a list (when necessary) should be pretty easily doable. But until we have actual use for a list, doing such a refactoring would kind of make awkward things like cache rebalancing. I guess that could be rephrased in terms of more general math… but why introduce the generality before we need it?

    instead of having to write special case pruning logic, this could be integrated with Improve Indices on pruned nodes via prune blockers #21726, and each chainstate could just add to a list of prune blockers

    Can we not do this under the current design? I don’t see why the suggested design changes would enable or simplify this, but admittedly I haven’t given that work a close look yet.

    Keeping future code maintainable and having confidence in completeness of the change.

    I think the reality is that introducing the possibility of multiple chainstates is a huge overhaul to the system and there’s no free lunch in terms of simplification. Unless we move a lot of domain-specific code into the chainman, we’re not going to get around some degree of scatter. I think in either case, you’re not going to be able to verify completeness without grepping for every CChainState usage and manually examining it; I don’t think there’s a way around this regardless of the design you choose.


    Grumbles aside I will spend the next few days trying to implement your proposed changes in an alternate branch. If the diff is small enough and I get your sign-off on completeness, I’ll replace HEAD here.

  243. jamesob force-pushed on Jun 28, 2021
  244. ryanofsky commented at 7:53 pm on June 28, 2021: contributor

    Thanks and to be sure I’m happy to review any version of this PR with or without the changes I was suggesting. My comment #15606#pullrequestreview-692965905 was trying to make sense of things for myself as much as it was trying to give suggestions for the PR. So definitely feel free to any skip extra implementation work unless you think it will actually improve things.

    But just to clarify, supporting multiple background chainstates was not my motivation at all. (I did try to think of use cases for multiple chainstates but couldn’t think of any that seemed very realistic.) My motivation was purely to make resulting code more understandable and maintainable. I think ideally assumeutxo phases would be encapsulated completely in the ChainstateManager class and not referenced in any other validation code, including ChainState methods. I think outside of ChainstateManager no other validation code that should have to know about the “IBD chainstate” “Snapshot chainstate” “Active chainstate” “Background IBD chainstate” or"Validated chainstate". The jargon still just blurs together for me. I think ideally validation code should just deal with chains and blocks based on their attributes, not based on assumeutxo stages or the global sync state. Things just seem less confusing and more reassuring that way (more reassuring because special cases are less special and because background chainstates don’t have access to the mempool).

  245. jamesob commented at 8:09 pm on June 28, 2021: member

    Thanks @ryanofsky, that’s a really helpful clarification for me. Agree that there is probably more of a proliferation of assumeutxo jargon than there needs to be. At the very least what I think I can do is consolidate terminology to “active” chainstate vs. “background” chainstate (… or maybe just non-“active”?). I’ll look at doing that in addition to your other recommendations.

    I’ve pushed a rebase (utxo-dumpload.81) that reintroduces https://github.com/bitcoin/bitcoin/pull/15606/commits/cf9366fa4ffb37fd07ef44f0e8495226dcd72aea which I thought I might be able to omit but apparently is required to avoid test failures in certain configurations. I’m going to dig into the particulars of the related error to reconfirm the necessity of the commit.

  246. ryanofsky commented at 8:37 pm on June 28, 2021: contributor

    In some cases switching behavior on mempool presence might be even more confusing (e.g. b51d6d7) since the mempool is basically unrelated to things like validationinterface callbacks.

    This is probably subjective, but I don’t see it as being unrelated. If the chain is connected to the mempool it’s active, and BlockConnected should be called instead of BackgroundBlockConnected, and rewinds and disconnects are possible instead of impossible, and other notifications should be sent too.

    Fundamentally a lot of the validation/net logic just needs to know whether it’s dealing with the active chainstate or not;

    There could be bool IsBackground() const { return m_mempool == nullptr; } or bool IsActive() const { return m_mempool != nullptr; } helpers maybe. I tend to not like vocabulary-introducing things like this, because then you have the mental overhead of putting cases into a taxonomy instead of just making all the code handle all the cases. But this is very subjective.

    I don’t see a way around that short of moving a maybe-inappropriate amount of domain-specific code into the ChainstateManager.

    I’m actually not sure what this would be referring to.

    On list-of-chainstates vs. m_ibd_chainstate and m_snapshot_chainstate, these are private members of chainman and so swapping them out for a list (when necessary) should be pretty easily doable. But until we have actual use for a list, doing such a refactoring would kind of make awkward things like cache rebalancing. I guess that could be rephrased in terms of more general math… but why introduce the generality before we need it?

    Funny, it seems to me more general, less hardcoded math would actually be simpler to understand. Take the cache, divide it up, maybe boost some chain’s weighting (I forget already) by some factor based on it’s attributes. No thinking about assumeutxo phases or terminology.

    instead of having to write special case pruning logic, this could be integrated with Improve Indices on pruned nodes via prune blockers #21726, and each chainstate could just add to a list of prune blockers

    Can we not do this under the current design? I don’t see why the suggested design changes would enable or simplify this, but admittedly I haven’t given that work a close look yet.

    Yes this would be doable under current design. I just mentioned it because it would be optional under current design, but required under the suggested design (because pruning code wouldn’t know about the types of chainstates, it would just have to treat them generically and ask what blocks they need to keep).

    Keeping future code maintainable and having confidence in completeness of the change.

    I think the reality is that introducing the possibility of multiple chainstates is a huge overhaul to the system and there’s no free lunch in terms of simplification. Unless we move a lot of domain-specific code into the chainman, we’re not going to get around some degree of scatter.

    I don’t think I’m suggesting to move any large pieces of code, but yes I do think ChainstateManager would be a little more complex because it would be more responsible for setting up and updating attributes of the chains so other validation code doesn’t have to know about assumeutxo. I think this is just how things go with encapsulation. You don’t necessarily reduce complexity, but you try locate it in one place.

    Again though everything here is a suggestion and you will have a much better idea of this stuff than me. I’ll take a look at the new branch, and I’m happy to review anything.

  247. maflcko commented at 1:12 pm on July 6, 2021: member
    Changing m_mempool from reference to pointer is something I wanted to do anyway for the -nomempool config option. If this also simplifies assumeutxo, I’d suggest doing it first. Happy to take a stab at this unless you are already working on it.
  248. jamesob commented at 1:34 pm on July 6, 2021: member

    Changing m_mempool from reference to pointer is something I wanted to do anyway for the -nomempool config option. If this also simplifies assumeutxo, I’d suggest doing it first. Happy to take a stab at this unless you are already working on it.

    Started looking at this last week and then America had a birthday. Should have a commit coming today; actually looks like a pretty small change.

  249. jamesob referenced this in commit 94e865e434 on Jul 7, 2021
  250. jamesob referenced this in commit b17bb4c37d on Jul 8, 2021
  251. jamesob referenced this in commit 8511429f3b on Jul 8, 2021
  252. jamesob referenced this in commit ad911753a1 on Jul 9, 2021
  253. jamesob referenced this in commit 04ac8b8e5c on Jul 12, 2021
  254. in src/validation.cpp:2536 in b63ff90d65 outdated
    2356@@ -2357,7 +2357,9 @@ bool CChainState::DisconnectTip(BlockValidationState& state, const CChainParams&
    2357     UpdateTip(m_mempool, pindexDelete->pprev, chainparams, *this);
    2358     // Let wallets know transactions went from 1-confirmed to
    2359     // 0-confirmed or conflicted:
    2360-    GetMainSignals().BlockDisconnected(pblock, pindexDelete);
    2361+    if (this == &m_chainman.ActiveChainstate()) {
    


    ryanofsky commented at 9:45 pm on July 12, 2021:

    In commit “validation: only send ValidationInterface callbacks for active chain” (b63ff90d6597d1d81b198f5d4d91551fe9ff75f2)

    Is disconnecting the tip possible if the chain is not active? I guess the if statement could be changed to an assert if it’s always supposed to be true. Or it could be good to have a comment to explain why there’s no else branch BackgroundBlockDisconnected call.

  255. in src/validation.cpp:5137 in b73572bc92 outdated
    5133@@ -5134,15 +5134,31 @@ void ChainstateManager::MaybeRebalanceCaches()
    5134     }
    5135 }
    5136 
    5137-std::optional<CBlockIndex*> ChainstateManager::GetSnapshotBaseBlock()
    5138+std::optional<unsigned int> ChainstateManager::GetSnapshotNChainTx()
    


    ryanofsky commented at 9:52 pm on July 12, 2021:

    In commit “wallet: avoid rescans if under the snapshot” (b73572bc92c07d246f322135ef9826f7e4cbf1d6)

    Is this meant to be part of a different commit? Seems unrelated to the wallet change

  256. in src/index/base.cpp:282 in ad65151497 outdated
    277+        // snapshot chainstate), look at only updating this when pindex's
    278+        // height is greater than best_block's.
    279+        //
    280+        // Note: I tried this earlier, but got some test failures
    281+        // (functional/rpc_rawtransactions.py and rpc_getblockfilter.py).
    282         m_best_block_index = pindex;
    


    ryanofsky commented at 9:57 pm on July 12, 2021:

    In commit “validation: indexing changes for assumeutxo” (ad65151497e0e7ddc2c7c7bfed854a6d0b30828f)

    Should this only be updated if a block is connected to the active chain, not the background chain?


    jamesob commented at 5:03 pm on July 17, 2021:

    I think if anything, probably the reverse; if we only do this accounting for the active chain, we run the risk of giving the impression that the index has been fully built up until that point (when in reality we’re waiting for block data to come in further back in the chain via the background chainstate).

    This requires more thought, but my inclination is to either (i) explicitly reform the assumption that indexes will be built sequentially, or (ii) disallow BlockConnected handling for the active chainstate when there is a background chainstate in use.

    I’ll add plenty of documentation around this in the next day or so. And I should probably try to write some tests for the interaction between indexation and assumeutxo use somehow.


    jamesob commented at 5:05 pm on July 17, 2021:
    Although it’s worth noting that BlockConnected signals are ignored by the indexer until a full indexation has completed, so this may be a moot point anyway (which I think is why I didn’t materially change the code in the first place…).

    jamesob commented at 5:33 pm on July 17, 2021:

    Although it’s worth noting that BlockConnected signals are ignored by the indexer until a full indexation has completed

    Hah, just kidding - turns out that m_synced flips to true in a degenerate case when we start up with an empty datadir; i.e. if a user enables -txindex=1 on first use. I’m not sure if jimpo intended this to happen when he wrote it, but because m_chain has no entries the latch flips immediately. So the above discussion is not moot, and this (maybe surprising) behavior should probably be documented in BaseIndex. I may do that in a separate PR soon.

  257. in src/validation.cpp:5638 in 8dc65c32ff outdated
    5221@@ -5222,3 +5222,13 @@ std::optional<int> ChainstateManager::GetSnapshotHeight()
    5222     }
    5223     return (*base)->nHeight;
    5224 }
    5225+
    5226+bool ChainstateManager::IsAnyChainInIBD()
    


    ryanofsky commented at 10:12 pm on July 12, 2021:

    In commit “p2p: don’t advertise until we finish all IBDs” (8dc65c32ffb533402bc0c82ccf3f1fe5535df521)

    It seems messy for individual chains have their own IBD states, and interact with shared fImporting and fReindex and nMinimumChainWork globals, and use their own latches.

    I think the IBD logic would be more straightforward if the CChainState::IsInitialBlockDownload method and CChainState::m_cached_finished_ibd member were moved from the CChainState class to the ChainstateManager class and consolidated in one place.

  258. ryanofsky commented at 10:28 pm on July 12, 2021: contributor

    re: #15606 (comment)

    Changing m_mempool from reference to pointer is something I wanted to do anyway for the -nomempool config option.

    I think it doesn’t change my suggestion too much, but if CChainState::m_mempool will also be null when -nomempool is used, m_mempool being null won’t automatically imply the chain is not the active chain, so a CChainState::m_is_active or CChainState::m_is_background member may also be necessary.

    #22415 implements the m_mempool change though.

  259. jamesob referenced this in commit 617661703a on Jul 13, 2021
  260. maflcko referenced this in commit c0224bc962 on Jul 15, 2021
  261. hebasto referenced this in commit 44bedb8d8a on Jul 19, 2021
  262. jarolrod referenced this in commit b9159a5c5c on Jul 19, 2021
  263. josibake referenced this in commit 737560d531 on Jul 21, 2021
  264. jamesob force-pushed on Jul 23, 2021
  265. jamesob force-pushed on Jul 23, 2021
  266. jamesob force-pushed on Jul 23, 2021
  267. jamesob force-pushed on Jul 24, 2021
  268. jamesob force-pushed on Jul 24, 2021
  269. in doc/assumeutxo.md:6 in e9e673950b outdated
    0@@ -0,0 +1,137 @@
    1+# assumeutxo
    2+
    3+Assumeutxo is a feature that allows fast bootstrapping of a validating bitcoind
    4+instance with a very similar security model to assumevalid.
    5+
    6+The RPC comanmds `dumptxoutset` and `loadtxoutset` are used to respectively generate
    


    ryanofsky commented at 6:16 pm on July 25, 2021:

    In commit “doc: add assumeutxo notes” (e9e673950b906957b9b691ac0efa3894463a0f92)

    spelling commands

  270. in src/chain.h:139 in e9e673950b outdated
    130+    BLOCK_OPT_WITNESS        =   128, //!< block data in blk*.data was received with a witness-enforcing client
    131+
    132+    /**
    133+     * If set, this indicates that the block index entry is assumed-valid.
    134+     * Certain diagnostics will be skipped in e.g. CheckBlockIndex().
    135+     * It almost certainly means that the block's full validation is pending
    


    ryanofsky commented at 7:11 pm on July 25, 2021:

    In commit “chain: add BLOCK_ASSUMED_VALID for use with assumeutxo” (0e8854e70374a7fff7efa32e13cb2effbd782f40)

    Is it possible to drop “almost certainly” here? If there is actually ambiguity, it should be possible to say what the ambiguity is.

  271. in src/index/base.cpp:264 in e9e673950b outdated
    252+    // Ignore BlockConnected signals until we have fully indexed the chain.
    253     if (!m_synced) {
    254         return;
    255     }
    256 
    257+    bool bg_chainstates_in_use = m_chainman->hasBgChainstateInUse();
    


    ryanofsky commented at 8:21 pm on July 25, 2021:

    In commit “validation: indexing changes for assumeutxo” (cba98bb776763e5ca8e290504004dadc8c48f30d)

    Why does indexing have to be aware of assumueutxo chainstates? In the abstract, it seems like indexes should only need to know:

    • When the tip is changing (to update best block pointer and to undo rewinds)
    • When new block data is available (to add to the index)

    If it’s true that indexing code already does not care about the order of blocks, then it would seem pretty straightforward for validation code to just send these two events to indexing API without indexing code having to change much, and without it having to know about assumeutxo chainstates.


    jamesob commented at 4:49 pm on July 29, 2021:

    The reason the indexing code needs to be aware of background chainstates is because the best block pointer (m_best_block_index) was (and is) used to signal that all blocks including and beneath that blockhash have been indexed. When you’ve got two separate chainstates throwing off BlockConnected events, you basically have to reserve updating that pointer for the background chainstate because otherwise you’re breaking the nature of the best block pointer. Updating the index when you get an assumed-valid chainstate event would mean that there are unindexed blocks beneath the pointer.

    I’ve tried to think about ways to avoid this situation, but haven’t been able to figure out anything simpler. If you’ve got ideas, I’m definitely curious and happy to implement.


    ryanofsky commented at 5:16 pm on July 29, 2021:

    Yeah I think what I’m asking is beyond the scope of the PR. It seems to me ideally validation code would notify indexing code “the tip is changing” and “here is some new block data” in 2 separate events not 3 mixed up events (block connected, background block connected, rewind), and also that indexing code would more passively receive data instead of querying chainstatemanager to reverse-engineer what validation code is doing.

    But probably what you’re doing now is the simplest approach for this PR.

  272. in src/index/txindex.cpp:214 in e9e673950b outdated
    203@@ -204,7 +204,13 @@ bool TxIndex::Init()
    204     // Attempt to migrate txindex from the old database to the new one. Even if
    205     // chain_tip is null, the node could be reindexing and we still want to
    206     // delete txindex records in the old database.
    207-    if (!m_db->MigrateData(*m_chainstate->m_blockman.m_block_tree_db, m_chainstate->m_chain.GetLocator())) {
    208+    //
    209+    // We can use the active chain here without consideration for multiple
    210+    // chainstates because if a complete txindex exists to be migrated, it
    211+    // implies we've already completed IBD.
    


    ryanofsky commented at 1:42 pm on July 26, 2021:

    In commit “validation: indexing changes for assumeutxo” (cba98bb776763e5ca8e290504004dadc8c48f30d)

    I think this comment would make more sense in the migratedata method as part of the commment describing how it works. Otherwise it’s not clear why there is any relationship between the database format and IBD, much less the relationship to multiple chainstates


    jamesob commented at 5:52 pm on July 29, 2021:
    That’s a good point; will move that over.

    jamesob commented at 6:12 pm on August 5, 2021:
    Fixed - I’ve left the comment here, but added one to the method.
  273. in src/init.cpp:1423 in e9e673950b outdated
    1361+                // If we're not using a snapshot or we haven't fully validated it yet,
    1362+                // create a validation chainstate.
    1363+                if (!chainman.IsSnapshotValidated()) {
    1364+                    LogPrintf("Loading validation chainstate\n");
    1365+                    chainman.InitializeChainstate(Assert(node.mempool.get()));
    1366+                }
    


    ryanofsky commented at 1:45 pm on July 26, 2021:

    In commit “add utxo snapshot detection and add to init” (9e277298d8529d1afb8743a6e67cc5a3363d2703)

    Could this logic be moved to chainstatemanager, maybe inside the InitializeChainstate. It would seem better for it to be encapsulated there and not have to be exposed to application init code.


    jamesob commented at 6:15 pm on August 5, 2021:
    I took a run at doing this (https://github.com/jamesob/bitcoin/commit/cec687abb4f587d756bbaac62c02c09704cd8a42), but the refactor necessary to encapsulate all of the chainstate init logic is fairly complicated and difficult to verify correctness for. I burned more time than I’d like to admit trying to get this to work, but was getting a weird race/segfault with the loadblk thread as a result of moving this stuff around. So rather than further bloat this changeset I’m going to leave this for a future PR.
  274. in src/net_processing.cpp:655 in e9e673950b outdated
    652+    //!
    653+    //! Nota bene: this is contingent on the ChainstateManager not destructing
    654+    //! any CChainState objects which were in use at any point (e.g. a background
    655+    //! validation chainstate which has completed) until the end of
    656+    //! init.cpp:Shutdown(), else we'll have bad pointers here.
    657+    std::map<const CChainState* const, const CBlockIndex*> chainstate_to_last_common_block = {};
    


    ryanofsky commented at 1:48 pm on July 26, 2021:

    In commit “net_processing: work with multiple chainstates” (39addc71a0c6f574acfc5e3fc6c9e75083530b81)

    const CChainState* const could be const CChainState* because map keys are always const


    jamesob commented at 6:24 pm on August 5, 2021:
    Fixed.
  275. ryanofsky commented at 2:20 pm on July 26, 2021: contributor

    Approach ACK e9e673950b906957b9b691ac0efa3894463a0f92. Thanks for all the updates! Need to review in more detail but the change really seems like it is getting small & manageable.

    Maybe can remove [experimental] tag? I think I saw a minor todo comment or two in net processing code, but otherwise it seems like there is not very much uncertain or left to be resolved here, unless I’m missing something.

    It could also be good to update the PR description, and because it seems like it is referencing changes that have already been made in prior PRs. You could just mention the PR numbers and say the text describes the combined changes.

  276. jamesob force-pushed on Aug 5, 2021
  277. jamesob force-pushed on Aug 5, 2021
  278. jamesob force-pushed on Aug 5, 2021
  279. jamesob commented at 6:25 pm on August 5, 2021: member
    Okay, I’ve pushed a rebase bringing this up to date with master and addressing a few of @ryanofsky’s comments. I think this is in pretty good shape.
  280. jamesob renamed this:
    [experimental] UTXO snapshots
    assumeutxo
    on Aug 5, 2021
  281. jamesob marked this as ready for review on Aug 5, 2021
  282. Sjors commented at 7:00 pm on August 6, 2021: member

    Fresh snapshot at height 694,000: magnet:?xt=urn:btih:d7216e8dd1b9dcf8c136f20156b966a745003015&dn=utxo%5Fmainnet%5F694000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969

    To use the snapshot, edit chainparams.cpp:

    0        m_assumeutxo_data = MapAssumeutxo{
    1            {
    2                694000,
    3                {AssumeutxoHash{uint256S("0x00000000000000000003f5acb5ec81df7c98c16bc8d89bdaadd4e8965729c018")}, 659945001},
    4            },
    5        };
    

    I’m able to load the snapshot and sync to the tip. I’ll let you know if the background sync fails.

    The “flushing snapshot chainstate to disk” could use some progress indicators in the log, but this PR is already big enough.

    I did notice that during the catchup, Taproot is marked as DEFINED rather than LOCKED_IN:

     0 "taproot": {
     1      "type": "bip9",
     2      "bip9": {
     3        "status": "defined",
     4        "start_time": 1619222400,
     5        "timeout": 1628640000,
     6        "since": 0,
     7        "min_activation_height": 709632
     8      },
     9      "active": false
    10    }
    

    You may want to doublecheck that you’re enforcing new consensus rules if a soft-fork activates after the snapshot.

    Update 2021-08-08: it synced fine. Taproot is now shown as locked_in. But when I start bitcoind again (minutes after shutting it down gracefully, doom:

     02021-08-08T10:36:05Z Cache configuration:
     12021-08-08T10:36:05Z * Using 2.0 MiB for block index database
     22021-08-08T10:36:05Z * Using 8.0 MiB for chain state database
     32021-08-08T10:36:05Z * Using 14990.0 MiB for in-memory UTXO set (plus up to 953.7 MiB of unused mempool space)
     42021-08-08T10:36:05Z init message: Loading block index…
     52021-08-08T10:36:05Z [snapshot] detected active snapshot chainstate ("/Users/sjors/.bitcoin-ext-hdd/chainstate_00000000000000000003f5acb5ec81df7c98c16bc8d89bdaadd4e8965729c018") - loading
     62021-08-08T10:36:05Z Switching active chainstate to Chainstate [snapshot] @ height -1 (null)
     72021-08-08T10:36:05Z Loading validation chainstate
     82021-08-08T10:36:05Z Opening LevelDB in /Users/sjors/.bitcoin-ext-hdd/blocks/index
     92021-08-08T10:36:05Z Opened LevelDB successfully
    102021-08-08T10:36:05Z Using obfuscation key for /Users/sjors/.bitcoin-ext-hdd/blocks/index: 0000000000000000
    112021-08-08T10:36:05Z Opening LevelDB in /Users/sjors/.bitcoin-ext-hdd/chainstate
    122021-08-08T10:36:06Z Opened LevelDB successfully
    132021-08-08T10:36:06Z Using obfuscation key for /Users/sjors/.bitcoin-ext-hdd/chainstate: 4285ede2cdd16ad3
    142021-08-08T10:36:06Z Opening LevelDB in /Users/sjors/.bitcoin-ext-hdd/chainstate_00000000000000000003f5acb5ec81df7c98c16bc8d89bdaadd4e8965729c018
    152021-08-08T10:36:06Z Opened LevelDB successfully
    162021-08-08T10:36:06Z Using obfuscation key for /Users/sjors/.bitcoin-ext-hdd/chainstate_00000000000000000003f5acb5ec81df7c98c16bc8d89bdaadd4e8965729c018: bd9dd8e1af78ac03
    172021-08-08T10:36:11Z 
    18
    19[snapshot] setting CBlockIndex(pprev=0x7f8944e6ab60, nHeight=694000, merkle=184538ca130f25e57570a0fa50793845da76f0f8cc2098824044d4e7eca397f5, hashBlock=00000000000000000003f5acb5ec81df7c98c16bc8d89bdaadd4e8965729c018) nChainTx to 659945001
    20
    212021-08-08T10:36:11Z LoadBlockIndexDB: last block file = 2680
    222021-08-08T10:36:11Z LoadBlockIndexDB: last block file info: CBlockFileInfo(blocks=82, size=61069735, heights=694689...694781, time=2021-08-07...2021-08-08)
    232021-08-08T10:36:11Z Checking all blk files are present...
    242021-08-08T10:36:11Z Initializing chainstate Chainstate [ibd] @ height -1 (null)
    25Assertion failed: (!setBlockIndexCandidates.empty()), function PruneBlockIndexCandidates, file validation.cpp, line 2505.
    26zsh: abort      src/bitcoind -datadir=/Users/sjors/.bitcoin-ext-hdd
    
  283. jamesob commented at 8:38 pm on August 17, 2021: member

    As always, thanks for testing @Sjors. We’ve discussed on IRC, but just so others can follow along: what’s happened here is you’ve listed the blockhash in your chainparams (0x00000000000000000003f5acb5ec81df7c98c16bc8d89bdaadd4e8965729c018) instead of the UTXO set hash (0x2f47dc1e089b623a4bea57e0752b776df9e6885904d7ff8b14911b94c2208ff4), and so consequently snapshot activation fails. You might have mistaken regular IBD for a snapshot sync-to-tip.

    However in doing this, you’ve uncovered a minor bug: if a snapshot fails to validate, it doesn’t activate but it does leave its datadir in place. On next startup, init would try to load this snapshot chainstate on the basis of the datadir existence but, because it was never actually activated, blockindex assertions don’t allow the load to happen. This error would have only occurred in practice if a user is given a malformed snapshot file or is manually tweaking chainparams values, as we are here.

    I’ve pushed a commit ensuring that teardown of the snapshot datadir happens should activation fail. An alternative to this would be setting some kind of block index state that controls snapshot load on startup, but I don’t think that’s necessary.

    The correct chainparams modification for the snapshot you generated is

    0        m_assumeutxo_data = MapAssumeutxo{
    1            ...
    2            {
    3                694000,
    4                {AssumeutxoHash{uint256S("0x2f47dc1e089b623a4bea57e0752b776df9e6885904d7ff8b14911b94c2208ff4")}, 659945001},
    5            },
    6        };
    

    I’ve tested syncing to network tip with the 694000 snapshot, shutting down and booting back up at various points during the process, and it works for me. Thanks again for testing and finding bugs!

  284. DrahtBot added the label Needs rebase on Sep 16, 2021
  285. laanwj referenced this in commit b7e3600815 on Sep 23, 2021
  286. jamesob force-pushed on Sep 30, 2021
  287. DrahtBot removed the label Needs rebase on Sep 30, 2021
  288. jamesob force-pushed on Oct 1, 2021
  289. jamesob force-pushed on Oct 1, 2021
  290. jamesob force-pushed on Oct 1, 2021
  291. DrahtBot added the label Needs rebase on Oct 5, 2021
  292. jamesob force-pushed on Oct 28, 2021
  293. jamesob force-pushed on Oct 28, 2021
  294. maflcko referenced this in commit 23ae7931be on Nov 3, 2021
  295. janus referenced this in commit 1729e5f47f on Nov 5, 2021
  296. jamesob force-pushed on Nov 9, 2021
  297. laanwj referenced this in commit 6acda4b00b on Dec 2, 2021
  298. PhotoshiNakamoto referenced this in commit cda98e1821 on Dec 11, 2021
  299. maflcko referenced this in commit b67115dd04 on Dec 15, 2021
  300. jamesob force-pushed on Dec 22, 2021
  301. DrahtBot removed the label Needs rebase on Dec 22, 2021
  302. DrahtBot added the label Needs rebase on Jan 2, 2022
  303. in src/validation.cpp:3856 in d354a93b83 outdated
    3848@@ -3800,7 +3849,11 @@ void BlockManager::PruneOneBlockFile(const int fileNumber)
    3849     setDirtyFileInfo.insert(fileNumber);
    3850 }
    3851 
    3852-void BlockManager::FindFilesToPruneManual(std::set<int>& setFilesToPrune, int nManualPruneHeight, int chain_tip_height)
    3853+void BlockManager::FindFilesToPruneManual(
    3854+    std::set<int>& setFilesToPrune,
    3855+    int nManualPruneHeight,
    3856+    int chain_tip_height,
    3857+    ChainstateManager& chainman)
    


    maflcko commented at 8:07 am on January 4, 2022:

    Sorry for causing the conflict, but BlockManager moved to blockstorage.cpp

    Also, what is the status here? Do you plan to split another piece off or will this go in as one cake now?

  304. jamesob force-pushed on Jan 5, 2022
  305. DrahtBot removed the label Needs rebase on Jan 5, 2022
  306. jamesob force-pushed on Jan 5, 2022
  307. jamesob force-pushed on Jan 5, 2022
  308. Sjors commented at 1:46 pm on January 6, 2022: member

    CI failure:

    0test  2022-01-05T21:03:35.417000Z TestFramework (ERROR): Assertion failed 
    1                                   Traceback (most recent call last):
    2                                     File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/test_framework/test_framework.py", line 132, in main
    3                                       self.run_test()
    4                                     File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/feature_assumeutxo.py", line 76, in run_test
    5                                       assert_equal(
    6                                     File "/private/var/folders/xx/vl5f934s6k927z1vyyl9cxth0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-apple-darwin/test/functional/test_framework/util.py", line 50, in assert_equal
    7                                       raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    8                                   AssertionError: not(2791b5e739503d03fb174d983ef4cf7ef9f905dabca0299f16ebb2cc06bc9b67 == b58bd896c7eb420f61d1a6f40a0cdee43853992fcb820a6f854ccc9db95b3e5e)
    
  309. jamesob commented at 7:11 pm on January 6, 2022: member

    AssertionError: not(2791b5e739503d03fb174d983ef4cf7ef9f905dabca0299f16ebb2cc06bc9b67 == b58bd896c7eb420f61d1a6f40a0cdee43853992fcb820a6f854ccc9db95b3e5e)

    Hm, well that’s concerning. Does anyone know if TestFramework.generatetoaddress and its underlying functions have changed in such a way that would explain that? To my knowledge none of the UTXO set hashing functionality has changed…

  310. jamesob commented at 7:35 pm on January 6, 2022: member

    Oh here we go, this is the cause: https://github.com/bitcoin/bitcoin/commit/2539980e1d74a637be5bcadb566263f903adffd1

    I’ll update the tests now.

  311. jamesob force-pushed on Jan 6, 2022
  312. jamesob force-pushed on Jan 7, 2022
  313. DrahtBot added the label Needs rebase on Jan 7, 2022
  314. jamesob force-pushed on Jan 7, 2022
  315. jamesob force-pushed on Jan 7, 2022
  316. jamesob force-pushed on Jan 7, 2022
  317. DrahtBot removed the label Needs rebase on Jan 7, 2022
  318. DrahtBot added the label Needs rebase on Jan 25, 2022
  319. in src/node/blockstorage.cpp:207 in e842488142 outdated
    245-           nPruneTarget/1024/1024, nCurrentUsage/1024/1024,
    246-           ((int64_t)nPruneTarget - (int64_t)nCurrentUsage)/1024/1024,
    247-           nLastBlockWeCanPrune, count);
    248+        LogPrint(BCLog::PRUNE, "Prune: (%s) target=%dMiB actual=%dMiB diff=%dMiB max_prune_height=%d removed %d blk/rev pairs\n",
    249+               chainstate->ToString(), nPruneTarget/1024/1024, nCurrentUsage/1024/1024,
    250+               ((int64_t)nPruneTarget - (int64_t)nCurrentUsage)/1024/1024,
    


    PastaPastaPasta commented at 1:17 pm on February 1, 2022:
    please convert these to c++11 style functional casts
  320. in src/rpc/blockchain.cpp:2952 in e842488142 outdated
    2851+        const CBlockIndex* tip = chain.Tip();
    2852+
    2853+        data.pushKV("blocks",                (int)chain.Height());
    2854+        data.pushKV("bestblockhash",         tip->GetBlockHash().GetHex());
    2855+        data.pushKV("difficulty",            (double)GetDifficulty(tip));
    2856+        data.pushKV("mediantime",            (int64_t)tip->GetMedianTimePast());
    


    PastaPastaPasta commented at 1:19 pm on February 1, 2022:
    please convert these three to c++11 style functional casts
  321. in src/validation.h:594 in e842488142 outdated
    571     }
    572 
    573     //! Destructs all objects related to accessing the UTXO set.
    574     void ResetCoinsViews() { m_coins_views.reset(); }
    575 
    576+    bool HasCoinsViews() { return (bool)m_coins_views; }
    


    PastaPastaPasta commented at 1:28 pm on February 1, 2022:

    please avoid c-style cast; use a functional cast or be explicit like

    0bool(m_coins_views)
    1m_couns_view != nullptr
    
  322. jamesob force-pushed on Feb 1, 2022
  323. DrahtBot removed the label Needs rebase on Feb 1, 2022
  324. DrahtBot added the label Needs rebase on Feb 15, 2022
  325. gades referenced this in commit 6a887db9d9 on Mar 11, 2022
  326. achow101 referenced this in commit 8d4a058ac4 on Jul 18, 2022
  327. sidhujag referenced this in commit c97b370419 on Jul 18, 2022
  328. achow101 referenced this in commit 6912a28f08 on Oct 13, 2022
  329. jamesob force-pushed on Nov 10, 2022
  330. DrahtBot removed the label Needs rebase on Nov 11, 2022
  331. DrahtBot added the label Needs rebase on Dec 5, 2022
  332. fanquake referenced this in commit 82903a7a8d on Jan 30, 2023
  333. sidhujag referenced this in commit 8ff3d69a6a on Jan 30, 2023
  334. jamesob force-pushed on Feb 28, 2023
  335. DrahtBot removed the label Needs rebase on Mar 1, 2023
  336. achow101 referenced this in commit d5e4f9a439 on Mar 7, 2023
  337. DrahtBot added the label Needs rebase on Mar 8, 2023
  338. jamesob force-pushed on Mar 8, 2023
  339. DrahtBot removed the label Needs rebase on Mar 8, 2023
  340. Sjors commented at 5:15 pm on March 9, 2023: member

    CI trips over:

    0src/validation.cpp:5721:1: 
    1error: 
    2return type 'const ChainstateRole' is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
    

    I made a snapshot at height 780,000 and then loaded it.

     0[
     1  {
     2    "height": 780050,
     3    "hash": "000000000000000000043cfdd430cbcf5d6cfa0c437ad5b808e1d45dc509387e",
     4    "branchlen": 771607,
     5    "status": "headers-only"
     6  },
     7  {
     8    "height": 8443,
     9    "hash": "00000000ee985adaa7242745800b7702d48558fbc517358be4fb597368f8b217",
    10    "branchlen": 0,
    11    "status": "active"
    12  }
    13]
    

    Perhaps this is because the assume valid block was (unrealistically) close to the tip?

    I used the commits in this PR, but cleanly rebased on #24008 (replacing the first 6 commits here), and with a commit to modify the assumed valid block.

    I was able to produce this behaviour with a fresh datadir.

    Update 10-3-2023: I just tried again and noticed a log message that I probably overlooked: [snapshot] bad snapshot content hash …. This ~puzzling too, since I got the same sha256 hash when dumping the snapshot on both macOS and Ubuntu~ the umpteenth time I hardcoded a block hash instead of the content hash, ~but~ and it explains why IBD just went on as normal.

    It made it to the tip and is now doing a background sync.

    One thing I noticed if that whenc syncing to the tip, it was showing cache=3.0MiB even though it was using ~15GB of ~cache~ RAM. This is strange, because #27008 is not part of this PR, so most of the RAM should have cleared up once the snapshot is loaded.

    ~Also worrying is that~ once it reaches the tip, it doesn’t immediately flush. (Update: that makes sense, because we recently flushed.) That causes the background sync to have very limited resources. It endup up flushing the cache when it reach 1751MB (which matches the coinstip cache of 818 MB plus my maxmempool of 1GB). (update: I think the problem is that it’s not correctly rebalancing)

    Strangely it says [snapshot] validated snapshot (811 MB) and [snapshot] (811 MB), which seems completely off since the snapshot should use about 12 GB (-dbcache was set high enough).

    Once I restart the node it creates a more reasonable split between the background ibd (15555 MB) and snapshot chainstate caches (818 MB).

    Another thing I noticed is that because it takes a while to load and process the snapshot, it triggers a timeout in p2p networking and we start querying DNS seeds again. Can be dealt with in a followup.

    Loading a wallet that last opened before the assumevalid block fails gracefully, with a clear instruction to try again after sync reaches the snapshot height. Currently the GUI gives no indication that such a background sync is going on, but that’s ok for now. The user must have used the RPC to trigger snapshot use, and loading an old wallet on fresh node is not a typical novice use case.

  341. in src/chainparams.cpp:168 in 1dc803c1bd outdated
    163@@ -164,7 +164,10 @@ class CMainParams : public CChainParams {
    164         };
    165 
    166         m_assumeutxo_data = MapAssumeutxo{
    167-         // TODO to be specified in a future patch.
    168+            {
    169+                685000,
    


    Sjors commented at 1:18 pm on March 10, 2023:

    Optimistically bumping the block, but also making sure I don’t keep putting the wrong numbers here:

    0/*height=*/780000,
    1{AssumeutxoHash{uint256S(/*txoutset_hash=*/"0x97d2989ab4c1b2cfb4eaa37788aeea08a3a6757eabd94ae9b7cdde07e79381da")},
    2/*nchaintx=*/812391117},
    

    Maybe also add a string initialiser to AssumeutxoHash to avoid the extra uint256S.

  342. in src/rpc/blockchain.cpp:2788 in 010419cec6 outdated
    2787@@ -2788,6 +2788,66 @@ static RPCHelpMan loadtxoutset()
    2788     };
    2789 }
    2790 
    2791+static RPCHelpMan monitorsnapshot()
    2792+{
    2793+return RPCHelpMan{
    2794+        "monitorsnapshot",
    2795+        "\nReturn information about UTXO snapshot status.\n",
    


    Sjors commented at 1:40 pm on March 10, 2023:
    It would be useful to have information about dbcache allocation (and usage) here.

    jamesob commented at 1:51 pm on May 6, 2023:
    Done!
  343. DrahtBot added the label Needs rebase on Mar 15, 2023
  344. jamesob force-pushed on Apr 19, 2023
  345. jamesob force-pushed on Apr 19, 2023
  346. DrahtBot added the label CI failed on Apr 19, 2023
  347. DrahtBot removed the label Needs rebase on Apr 19, 2023
  348. jamesob force-pushed on Apr 19, 2023
  349. jamesob force-pushed on May 1, 2023
  350. jamesob force-pushed on May 1, 2023
  351. jamesob force-pushed on May 1, 2023
  352. jamesob force-pushed on May 3, 2023
  353. jamesob force-pushed on May 4, 2023
  354. jamesob force-pushed on May 4, 2023
  355. jamesob force-pushed on May 4, 2023
  356. DrahtBot removed the label CI failed on May 4, 2023
  357. jamesob force-pushed on May 5, 2023
  358. jamesob commented at 2:33 am on May 5, 2023: member

    Alright! This is fully rebased and passing CI. Copy/pasteable testing instructions follow.

    I’m pretty sure these are the complete changes necessary to enable use of assumeutxo (via loadtxoutset), including pruning and index use. It’s worth noting that 200 of these lines are an elaborate demo shell script and another good chunk of this is airy RPC code, so this 1200 line diff is maybe more approachable than it appears.

    Recent changes

    • I’ve segmented blockfile storage into “assumed” vs. “normal” files to avoid doing drastic mixing of heights. As the code was written, we had snapshot and ibd chainstates pushing into the same space of blockfiles, which makes pruning ineffectual for a decent chunk of the chain. This is now fixed.
    • Ironed out all the indexing issues in a relatively simple way. Indexation now happens strictly sequentially, along the background chainstate (which is just a continuation of the “regular” chainstate that we initialize with before running loadtxoutset and creating the snapshot chainstate). Validationinterface events in the indexers are ignored for the snapshot chainstate; once we complete background validation, the indexers are restarted and continue (sequentially) into the previously unindexed region of the chain.
    • I’ve renamed monitorsnapshot (a utility RPC) to getchainstates.
    • Lotta rebase and bug fixes.

    Testing

    For fun (~5min)

    If you want to do a quick test, you can run ./contrib/devtools/test_utxo_snapshots.sh and follow the instructions. This is mostly obviated by the functional tests, though.

    For real (longer)

    If you’d like to experience a real usage of assumeutxo, you can do that too. I’ve cut a new snapshot at height 788'000 (https://img.jameso.be/utxo-788000.dat - but you can do it yourself with ./contrib/devtools/utxo_snapshot.sh if you want). Download that, and then create a datadir for testing:

     0$ cd ~/src/bitcoin  # or whatever
     1
     2# get the snapshot
     3$ curl https://img.jameso.be/utxo-788000.dat > utxo-788000.dat
     4
     5# you'll want to do this if you like copy/pasting 
     6$ export AU_DATADIR=/home/${USER}/au-test # or wherever
     7
     8$ mkdir ${AU_DATADIR}
     9$ vim ${AU_DATADIR}/bitcoin.conf
    10
    11dbcache=8000  # or, you know, something high
    12blockfilterindex=1
    13coinstatsindex=1
    14prune=3000
    15logthreadnames=1
    

    Obtain this branch, build it, and then start bitcoind:

    0$ git remote add jamesob https://github.com/jamesob/bitcoin
    1$ git fetch jamesob utxo-dumpload-compressed
    2$ git checkout jamesob/utxo-dumpload-compressed
    3
    4$ ./configure $conf_args && make  # (whatever you like to do here)
    5
    6# start 'er up and watch the logs
    7$ ./src/bitcoind -datadir=${AU_DATADIR}
    

    Then, in some other window, load the snapshot

    0$ ./src/bitcoin-cli -datadir=${AU_DATADIR} loadtxoutset $(pwd)/utxo-788000.dat
    

    You’ll see some log messages about headers retrieval and waiting to see the snapshot in the headers chain. Once you get the full headers chain, you’ll spend a decent amount of time (~10min) loading the snapshot, checking it, and flushing it to disk. After all that happens, you should be syncing to tip in pretty short order, and you’ll see the occasional [background validation] log message go by.

    In yet another window, you can check out chainstate status with

    0$ ./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
    

    as well as usual favorites like getblockchaininfo.

    Have fun and hope it works!

    Known weird behavior

    • The last few blocks to get the snapshot chainstate to tip seem to take a while to roll in… I’m not sure what’s going on here. It eventually syncs, but not as fast as I would think.
    • If you restart the node in the middle of the bg sync, nChainTx for the snapshot chain will not properly repopulate and your progress= for the snapshot chainstate will be weird. I think there’s an easy fix for this, which I’m investigating.

    Next steps

    Maybe I’ll open a PR that’s this changeset but without the pruning changes, as suggested by @sdaftuar. If we decide it’s preferable to wait on pruning we can, but I think the LOC delta is only maybe a hundred lines or so. Conceptually the diff might be bigger of course. Curious what others think here, given this (relatively complete?) set of changes.

  359. fanquake commented at 11:08 am on May 5, 2023: member

    Known weird behavior

    Runing through your steps above, everything seems to be working, except that my mempool was empty until I stopped and restarted bitcoind?

    If you restart the node in the middle of the bg sync, nChainTx for the snapshot chain will not properly repopulate and your progress= for the snapshot chainstate will be weird. I think there’s an easy fix for this, which I’m investigating.

    Yea. After a restart:

     0# ./src/bitcoin-cli -datadir=${AU_DATADIR} getblockchaininfo
     1...
     2  "chain": "main",
     3  "blocks": 788360,
     4  "headers": 788360,
     5  "bestblockhash": "00000000000000000004adb413e56a36a384f6a2a5b2070919400a5589e5e61c",
     6  "difficulty": 48005534313578.78,
     7  "time": 1683282690,
     8  "mediantime": 1683281530,
     9  "verificationprogress": 0.01514891955399454,
    10
    11# ./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
    12{
    13  "active_chain_type": "snapshot",
    14  "ibd": {
    15    "blocks": 217772,
    16    "bestblockhash": "00000000000003c6630bb58c5dfab652c2b921d850f9f9472d87739857b6f14b",
    17    "difficulty": 2968775.332075074,
    18    "verificationprogress": 0.01394596823378951,
    19    "snapshot_blockhash": "0000000000000000000000000000000000000000000000000000000000000000",
    20    "initialblockdownload": true,
    21    "coins_db_cache_bytes": 7969177,
    22    "coins_tip_cache_bytes": 8931455795
    23  },
    24  "snapshot": {
    25    "blocks": 788360,
    26    "bestblockhash": "00000000000000000004adb413e56a36a384f6a2a5b2070919400a5589e5e61c",
    27    "difficulty": 48005534313578.78,
    28    "verificationprogress": 0.0151489113147836,
    29    "snapshot_blockhash": "00000000000000000001f3fa1b4c03c877740778f56b0d5456b18dd88f7f695e",
    30    "initialblockdownload": false,
    31    "coins_db_cache_bytes": 419430,
    32    "coins_tip_cache_bytes": 470076620
    33  },
    34  "headers": 788360
    35}
    
  360. jamesob commented at 12:56 pm on May 5, 2023: member

    Thanks for testing!

    except that my mempool was empty until I stopped and restarted bitcoind?

    Interesting, I’ll look at this today; probably some part of net_processing that’s looking at chainman.IsAnyIBD() when it should be using ActiveChainstate().IsInitialBlockDownload().

  361. DrahtBot added the label Needs rebase on May 5, 2023
  362. validation: introduce ChainstateManager::GetChainstateForNewBlock
    and use it in ProcessNewBlock().
    
    This is needed for an upcoming change to `net_processing`.
    862346f870
  363. validation: add ChainstateManager::GetAllForBlockDownload()
    For use in subsequent changes to net_processing.
    89fe4b3a2c
  364. doc: add docstring for CanDirectFetch()
    Unrelated to assumeutxo changes, but while I was in the
    neighborhood...
    4b68a3a85f
  365. net_processing: work with multiple chainstates
    Most easily reviewed with `--ignore-space-change`
    
    - Modify FindNextBlocksToDownload() to parameterize the chainstate
      being worked on.
    
    - Change GetNodeStateStats to take the max nCommonHeight per peer across
      all chainstates.
    
    - Add CNodeState::chainstate_to_last_common_block
      * we need this to allow handling for a single peer to distinguish
        between separate chainstates we're simultaneously downloading blocks for
    
    - Share `requests_available` across chainstates when finding the next blocks
      to download (call to `FindNextBlocksToDownload()`).
    86d5cfdd78
  366. p2p: don't advertise until we finish all IBDs
    Until we have completed IBD on all chainstates, don't answer getheaders
    requests from our peers or advertise ourselves. This preserves the
    existing behavior of not performing these actions until IBD of the
    whole chain completes.
    a966a54df5
  367. net_processing: add commentary describing IsIBD use
    For all remaining usages of IsInitialBlockDownload() where
    the status of any background validation chainstate that might
    be in use isn't relevant.
    84a7c15ef8
  368. assumeutxo: disallow -reindex when background syncing
    We can't support reindexing while a background sync is happening because
    we don't yet have a contiguous chain -- we have two disjoint regions
    of the chain (bg and assumedvalid).
    1899c76609
  369. validation: MaybeRebalanceCaches when chain leaves IBD
    Check to see if we need to rebalance caches across chainstates when
    a chain leaves IBD.
    5cdc31a252
  370. validation: add ChainstateRole fb817f71e8
  371. validation: pass ChainstateRole for validationinterface calls
    This allows consumers to decide how to handle events from background or
    assumedvalid chainstates.
    ccba1d10ba
  372. validationinterface: only send zmq notifications for active 70ff045a91
  373. wallet: validationinterface: only handle active chain notifications c9e1dfc0fe
  374. net_processing: validationinterface: ignore some events for bg chain 71dc7f44f4
  375. index: cache indexer references on ChainstateManager
    This is done for use in later commits. Once snapshot validation
    completes, we must restart the indexers so that they continue
    indexation (sequentially) on the fully validated snapshot chainstate
    now that all the requisite block data is there.
    b90b7fda3a
  376. validation: indexing changes for assumeutxo
    When using an assumedvalid chainstate, only process validationinterface
    callbacks from the background chainstate within indexers. This ensures
    that all indexes are built in-order.
    
    Later, we can possibly designate indexes which can be built out of order
    and continue their operation during snapshot use.
    
    Once the background sync has completed, restart the indexers so that
    they continue to index the now-validated snapshot chainstate.
    716fc95509
  377. validation: pruning for multiple chainstates
    Introduces ChainstateManager::GetPruneRange().
    9e8f6c093f
  378. blockstorage: segment normal/assumedvalid blockfiles
    When using an assumedvalid (snapshot) chainstate along with a background
    chainstate, we are syncing two very different regions of the chain
    simultaneously. If we use the same blockfile space for both of these
    syncs, wildly different height blocks will be stored alongside one
    another, making pruning ineffective.
    
    This change implements a separate blockfile cursor for the assumedvalid
    chainstate when one is in use.
    248eb32f58
  379. validation: run CheckBlockIndex on all chainstates during ProcessNewHeaders ddf2699ade
  380. rpc: add loadtxoutset 8d24f9b478
  381. Sjors commented at 6:12 pm on May 5, 2023: member
    Here’s a torrent for the snapshot: magnet:?xt=urn:btih:a457a54c76dfdbb3f44e3485a84c4772bea647e0&dn=utxo-788000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969
  382. rpc: add getchainstates 9fba087987
  383. test: add feature_assumeutxo functional test 0a191bddbf
  384. contrib: add script to demo/test assumeutxo
    Add the script to the shellcheck exception list since the
    quoted variables rule needs to be violated in order to get
    bitcoind to pick up on $CHAIN_HACK_FLAGS.
    63aac5055f
  385. doc: update release-process.md for assumeutxo 492cb79140
  386. chainparams: add assumeutxo param at height 788_000 3f97b27c17
  387. validation: populate nChainTx value for assumedvalid chainstates
    Use the expected AssumeutxoData in order to bootstrap nChainTx values
    for assumedvalid blockindex entries in the snapshot chainstate. This
    is necessary because nChainTx is normally built up from nTx values,
    which are populated using blockdata which the snapshot chainstate
    does not yet have.
    55267889cf
  388. validation: assumeutxo: swap m_mempool on snapshot activation
    Otherwise we will not receive transactions during background sync until
    restart.
    7cbbf2adad
  389. jamesob force-pushed on May 5, 2023
  390. DrahtBot removed the label Needs rebase on May 5, 2023
  391. fanquake commented at 10:57 am on May 6, 2023: member

    Background sync finshed catching up:

     02023-05-06T06:42:11Z [msghand] UpdateTip: new best=0000000000000000000104bcc48cdaaf080653751bfe4df10d2097f860abedd0 height=788463 version=0x30296000 log2_work=94.162512 tx=12944905 date='2023-05-06T06:40:07Z' progress=0.015582 cache=127.2MiB(988602txo)
     12023-05-06T06:46:56Z [msghand] [background validation] UpdateTip: new best=0000000000000000000454bc0c2b24c93b359d5eba2cf98d0108e5c748771ef7 height=786000 version=0x2eef6000 log2_work=94.128679 tx=825161729 date='2023-04-18T17:19:01Z' progress=0.993293 cache=268.5MiB(2211076txo)
     22023-05-06T06:47:16Z [msghand] New outbound peer connected: version: 70015, blocks=788463, peer=958 (outbound-full-relay)
     3<snip>
     42023-05-06T06:52:50Z [msghand] New outbound peer connected: version: 70016, blocks=788463, peer=973 (block-relay-only)
     52023-05-06T06:55:24Z [msghand] [background validation] UpdateTip: new best=00000000000000000001f3fa1b4c03c877740778f56b0d5456b18dd88f7f695e height=788000 version=0x29e9e000 log2_work=94.156236 tx=831186441 date='2023-05-02T22:16:30Z' progress=0.998719 cache=307.0MiB(2318605txo)
     62023-05-06T06:55:26Z [msghand] [snapshot] computing UTXO stats for background chainstate to validate snapshot - this could take a few minutes
     72023-05-06T06:56:21Z [msghand] [snapshot] snapshot beginning at 00000000000000000001f3fa1b4c03c877740778f56b0d5456b18dd88f7f695e has been fully validated
     82023-05-06T06:56:21Z [msghand] [snapshot] allocating all cache to the snapshot chainstate
     92023-05-06T06:56:21Z [msghand] Opening LevelDB in /Users/*/Downloads/assume_test/chainstate_snapshot
    102023-05-06T06:56:21Z [msghand] Opened LevelDB successfully
    112023-05-06T06:56:21Z [msghand] Using obfuscation key for /Users/*/Downloads/assume_test/chainstate_snapshot: ee33826b0d50cd61
    122023-05-06T06:56:21Z [msghand] [Chainstate [snapshot] @ height 788463 (0000000000000000000104bcc48cdaaf080653751bfe4df10d2097f860abedd0)] resized coinsdb cache to 8.0 MiB
    132023-05-06T06:56:21Z [msghand] [Chainstate [snapshot] @ height 788463 (0000000000000000000104bcc48cdaaf080653751bfe4df10d2097f860abedd0)] resized coinstip cache to 8966.0 MiB
    142023-05-06T06:56:22Z [scheduler] ChainStateFlushed: WARNING: Locator contains block (hash=00000000000000000001f3fa1b4c03c877740778f56b0d5456b18dd88f7f695e) not on known best chain (tip=000000000000000000025dbf6c4aab8e1dee993455b628eef315696e5fe4ef2f); not writing index locator
    152023-05-06T06:56:22Z [scheduler] ChainStateFlushed: WARNING: Locator contains block (hash=00000000000000000001f3fa1b4c03c877740778f56b0d5456b18dd88f7f695e) not on known best chain (tip=000000000000000000025dbf6c4aab8e1dee993455b628eef315696e5fe4ef2f); not writing index locator
    162023-05-06T06:56:22Z [basic block filter index] basic block filter index thread start
    172023-05-06T06:56:22Z [basic block filter index] Syncing basic block filter index with block chain from height 787644
    182023-05-06T06:56:22Z [coinstatsindex] coinstatsindex thread start
    192023-05-06T06:56:22Z [coinstatsindex] Syncing coinstatsindex with block chain from height 787644
    202023-05-06T06:56:32Z [basic block filter index] basic block filter index is enabled at height 788463
    212023-05-06T06:56:32Z [basic block filter index] basic block filter index thread exit
    222023-05-06T06:56:52Z [coinstatsindex] Syncing coinstatsindex with block chain from height 788246
    232023-05-06T06:57:03Z [coinstatsindex] coinstatsindex is enabled at height 788463
    242023-05-06T06:57:03Z [coinstatsindex] coinstatsindex thread exit
    252023-05-06T06:57:08Z [msghand] New outbound peer connected: version: 70016, blocks=788463, peer=974 (block-relay-only)
    262023-05-06T06:59:29Z [msghand] New outbound peer connected: version: 70016, blocks=788463, peer=975 (block-relay-only)
    272023-05-06T07:15:50Z [msghand] Saw new header hash=00000000000000000005a1fa7ff860245e4ec12b09a61618e0fbb44f2ebb1e5d height=788464
    282023-05-06T07:15:50Z [msghand] [net] Saw new cmpctblock header hash=00000000000000000005a1fa7ff860245e4ec12b09a61618e0fbb44f2ebb1e5d peer=970
    292023-05-06T07:15:50Z [msghand] UpdateTip: new best=00000000000000000005a1fa7ff860245e4ec12b09a61618e0fbb44f2ebb1e5d height=788464 version=0x2817c000 log2_work=94.162526 tx=12948693 date='2023-05-06T07:15:36Z' progress=0.015587 cache=128.6MiB(1000903txo)
    30<snip>
    312023-05-06T10:40:52Z [msghand] New outbound peer connected: version: 70016, blocks=788495, peer=1011 (block-relay-only)
    322023-05-06T10:46:45Z [msghand] Saw new header hash=0000000000000000000091d6b178462227c1334c10f14c39255160c0fc82875b height=788496
    332023-05-06T10:46:45Z [msghand] [net] Saw new cmpctblock header hash=0000000000000000000091d6b178462227c1334c10f14c39255160c0fc82875b peer=970
    342023-05-06T10:46:46Z [msghand] UpdateTip: new best=0000000000000000000091d6b178462227c1334c10f14c39255160c0fc82875b height=788496 version=0x20000000 log2_work=94.162955 tx=13090510 date='2023-05-06T10:46:26Z' progress=0.015756 cache=155.3MiB(1211908txo)
    
     0# ./src/bitcoin-cli -datadir=${AU_DATADIR} getchainstates
     1{
     2  "active_chain_type": "validated_snapshot",
     3  "validated_snapshot": {
     4    "blocks": 788496,
     5    "bestblockhash": "0000000000000000000091d6b178462227c1334c10f14c39255160c0fc82875b",
     6    "difficulty": 48005534313578.78,
     7    "verificationprogress": 0.01575630722365423,
     8    "snapshot_blockhash": "00000000000000000001f3fa1b4c03c877740778f56b0d5456b18dd88f7f695e",
     9    "initialblockdownload": false,
    10    "coins_db_cache_bytes": 8388608,
    11    "coins_tip_cache_bytes": 9401532416
    12  },
    13  "headers": 788496
    14}
    
  392. jamesob commented at 1:24 pm on May 6, 2023: member

    Thanks for testing @fanquake and thanks for the torrent @Sjors. I’ve pushed some fixes, rebased, and CI is green.

    Runing through your steps above, everything seems to be working, except that my mempool was empty until I stopped and restarted bitcoind?

    Fixed in https://github.com/bitcoin/bitcoin/pull/15606/commits/7cbbf2adad5a7760d6db389790062fee959943a6 - forgot to swap the m_mempool references on snapshot activation. I’ve verified that the mempool now starts to populate during background sync without a restart.

    If you restart the node in the middle of the bg sync, nChainTx for the snapshot chain will not properly repopulate

    Fixed this in https://github.com/bitcoin/bitcoin/pull/15606/commits/55267889cf4c3dbee8b62c0981f1469d87a7abba. Verified that getchainstates reports the snapshot chianstate progress as expected after restart.

  393. jamesob commented at 1:41 pm on May 6, 2023: member

    The history of the pull request is getitng unwieldy; the 400+ comments are now creating a situation where comments (like the testing instructions) posted a few days ago are buried under a minute of clicking “load more.”

    I’ve added the testing instructions to the PR description, but should I consider opening a fresh PR? Do we have any process for dealing with this Github limitation?

  394. fanquake commented at 1:45 pm on May 6, 2023: member
    @jamesob I would be in favour of you opening a new PR (carrying over relevant current context into the description, and pointing back to anything else relevant). I ran into the same annoyance today, when trying to leave my most recent comment. Having to expand 400+ comments check recent discussion/context, is not great.
  395. Sjors commented at 5:00 pm on May 6, 2023: member

    I also ended up with an empty mempool with 2dd8ea0f2cf07486d1a44263c6976b1ecac563b9 on Ubuntu 23.04, with pruning enabled. Will try again once you believe that’s fixed. When I (cleanly) shut down the node and restarted it, I noticed this log message: [snapshot] computing UTXO stats for background chainstate to validate snapshot - this could take a few minutes, followed by [snapshot] snapshot beginning at 00000000000000000001f3fa1b4c03c877740778f56b0d5456b18dd88f7f695e has been fully validated. Stopping and starting the node again that message no longer appeared and things seem normal.

    I also compared gettxoutsetinfo muhash for a recent and an old height to one of my other nodes, and they returned the same muhash, which suggests that indexes work.

  396. Sjors commented at 5:01 pm on May 6, 2023: member

    It’s gotten to the point where even refreshing the page doesn’t always get you the latest comments. +1 for opening a new one.

    Meanwhile I’ll recompile and do an unpruned with assumevalid=0.

  397. Sjors commented at 5:05 pm on May 6, 2023: member

    On Ubuntu 23.04 (gcc 12.2.0) I get a bunch of these warnings, that I don’t get on master:

    0In file included from ./wallet/wallet.h:10,
    1                 from wallet/dump.cpp:10:
    2./interfaces/chain.h:274:22: warning: ‘virtual void interfaces::Chain::Notifications::blockConnected(const interfaces::BlockInfo&)’ was hidden [-Woverloaded-virtual]
    3  274 |         virtual void blockConnected(const BlockInfo& block) {}
    
  398. jamesob commented at 3:09 pm on May 8, 2023: member

    Closing this as replaced by #27596.

    I get a bunch of these warnings, that I don’t get on master:

    Thanks for spotting this @Sjors; one-line removal fixed in the new PR.

  399. jamesob closed this on May 8, 2023

  400. achow101 referenced this in commit e7b0004b37 on Oct 2, 2023
  401. bitcoin locked this on May 7, 2024

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-07-01 10:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me