Validation: Remove ConnectTrace and PerBlockConnectTrace #17562

pull jnewbery wants to merge 1 commits into bitcoin:master from jnewbery:2019-11-remove-connect-trace changing 2 files +15 −61
  1. jnewbery commented at 7:27 PM on November 22, 2019: member

    ConnectTrace was introduced in 6fdd43b968f984ef92ca4576872dc65462ba7312 when the SyncTransactions signal needed to be fired outside the cs_main scope. The CValidationInterface is now asynchronous and signals can be fired while holding cs_main. Remove the ConnectTrace struct that needs to be passed up and down the stack and just fire BlockConnected() directly in ConnectTip().

  2. fanquake added the label Validation on Nov 22, 2019
  3. DrahtBot commented at 11:42 PM on November 22, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  4. ariard commented at 8:28 PM on November 25, 2019: member

    Concept ACK.

    Not sure about the wording of commit message, signals can be fired while holding cs_main, would say something more like callbacks can be scheduled while holding cs_main, at a later timer, the scheduler thread may execute them without regard to holding cs_main or not.

  5. jnewbery commented at 2:21 PM on November 26, 2019: member

    Thanks @ariard . I agree with you about the commit message. I've reworded along the lines you suggested.

  6. jnewbery force-pushed on Nov 26, 2019
  7. jnewbery renamed this:
    Validation: Remove ConnectTrace and PerBlockConnectTrace
    WIP: Validation: Remove ConnectTrace and PerBlockConnectTrace
    on Jan 2, 2020
  8. DrahtBot added the label Needs rebase on Jan 9, 2020
  9. in src/validation.cpp:2560 in aa882b24c9 outdated 
    2519 | @@ -2562,7 +2520,7 @@ bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& ch
2520 |      LogPrint(BCLog::BENCH, "  - Connect postprocess: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime6 - nTime5) * MILLI, nTimePostConnect * MICRO, nTimePostConnect * MILLI / nBlocksTotal);
2521 |      LogPrint(BCLog::BENCH, "- Connect block: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime6 - nTime1) * MILLI, nTimeTotal * MICRO, nTimeTotal * MILLI / nBlocksTotal);
2522 |  
2523 | -    connectTrace.BlockConnected(pindexNew, std::move(pthisBlock));
2524 | +    GetMainSignals().BlockConnected(pthisBlock, pindexNew);
    jonatack commented at 2:10 PM on January 22, 2020:

    If you need to retouch this PR: perhaps add code documentation here like that for GetMainSignals().BlockDisconnected (or GetMainSignals().ChainStateFlushed or GetMainSignals().UpdatedBlockTip)

    git grep -C1 "0-confirmed or conflicted"
  10. jonatack commented at 2:13 PM on January 22, 2020: member

    Concept ACK.

    Nice simplification and code removal that builds on the work in #17477.

  11. fjahr commented at 4:57 PM on January 22, 2020: member

    Concept ACK. Change seems fine but will wait with code review until #17477 is finalized and merged.

  12. jnewbery commented at 8:57 PM on March 13, 2020: member

    Closing for now. No need for this to be open while #17477 is not yet merged. Will reopen when #17477 is merged.

  13. jnewbery closed this on Mar 13, 2020
  14. [validation] Remove ConnectTrace and PerBlockConnectTrace
    ConnectTrace was introduced in 6fdd43b968f984ef92ca4576872dc65462ba7312
when the SyncTransactions signal needed to be fired outside the cs_main
scope. The CValidationInterface is now asynchronous and callbacks can be
scheduled without holding cs_main. The scheduler thread can execute the
callback later without regard to cs_main.

Remove the ConnectTrace struct that needs to be passed up and down the
stack and just fire BlockConnected() directly in ConnectTip().
    ab00be08e8
  15. jnewbery reopened this on Mar 19, 2020
  16. jnewbery force-pushed on Mar 19, 2020
  17. MarcoFalke commented at 9:30 PM on March 19, 2020: member

    Run ci

  18. MarcoFalke closed this on Mar 19, 2020
  19. MarcoFalke reopened this on Mar 19, 2020
  20. MarcoFalke commented at 9:32 PM on March 19, 2020: member

    There is a GitHub corruption: GitHub payload is missing a merge commit (mergeable_state: "blocked", merged: false)

  21. MarcoFalke closed this on Mar 19, 2020
  22. MarcoFalke reopened this on Mar 19, 2020
  23. MarcoFalke commented at 9:33 PM on March 19, 2020: member

    Ok, this was stupid. Ci is running now

    Concept ACK

  24. DrahtBot removed the label Needs rebase on Mar 19, 2020
  25. jnewbery renamed this:
    WIP: Validation: Remove ConnectTrace and PerBlockConnectTrace
    Validation: Remove ConnectTrace and PerBlockConnectTrace
    on Mar 22, 2020
  26. jnewbery commented at 12:58 AM on March 22, 2020: member

    #17477 is now merged. Re-opened this PR and rebased it on master.

  27. in src/validation.cpp:2537 in ab00be08e8 
    2532 | -    std::vector<PerBlockConnectTrace>& GetBlocksConnected() {
2533 | -        // We always keep one extra block at the end of our list because
2534 | -        // blocks are added after all the conflicted transactions have
2535 | -        // been filled in. Thus, the last entry should always be an empty
2536 | -        // one waiting for the transactions from the next block. We pop
2537 | -        // the last entry here to make sure the list we return is sane.
    MarcoFalke commented at 6:00 PM on March 23, 2020:

    Note to other reviewers: This comment obviously no longer applies after 312d27b11ceaacd3be75a14907f856352b7dcf48

  28. MarcoFalke approved
  29. MarcoFalke commented at 6:58 PM on March 23, 2020: member

    Removing code is fine, but I'd like to see the documentation updated a bit more. This will also explain why it is safe to reorganize the signals as a result of the changes in this pull request.

    Concept ACK ab00be08e8e4371bf1483d139ecb50dacba40e0a, removing code is my favorite! Though, needs some doc updated, I think 🎌

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

Concept ACK ab00be08e8e4371bf1483d139ecb50dacba40e0a, removing code is my favorite! Though, needs some doc updated, I think 🎌
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUi1Xwv8C0bXBK8kwoBLkKTv34M+YT29ESnNrB2kRI95LUZ1J/sXyzIR7nU39wZz
y+/w7cGwxVAITspBSctzKsu42BmrQLs9h4YVYLZLJEmoybHggrvm6c331xMwPexQ
Ktcozmee8qH5i+B0Pqyusk39Jnq/j6S2pm9ahgCh0woypucZWh27Rrjfg/YAgV6i
tbnhe0SY/FFffCH2d9vCFy/HPfPy+ZUSKQOLjY06P+DAJA7NBFzmddKMR7pOp+NB
jGA4RzcqYqACBmjh8WabTWoDf2hgYMyklMYe/HyaUE0Vg1WtYjJf3qapeeFtkMfx
iiGoO+HiASHvqi18Q0489o6nuDhWCIqpKkXl18+hdWvSbiNkySqd7KeWfPEU6s9G
SbEuImgxMJa7KbWkPlsjZxJKPStj8EP9zQr1NaVKt85E+MTCQAgrLj5kZL0/g9Ra
IOfg2RntWbOOqyE3AlT376IAk3sMvhzJZDxWQGhbRMQ4ew1ut+phojsylFGDNyVB
pJVLAPyQ
=fKV2
-----END PGP SIGNATURE-----

    Timestamp of file with hash 33cca684633289f50bf74baf0a39711a82c5a0b6fcd6d62d0dc47653f74fa2fa -

    </details>

  30. in src/validation.cpp:2508 in ab00be08e8 
    2546 |   * corresponding to pindexNew, to bypass loading it again from disk.
2547 | - *
2548 | - * The block is added to connectTrace if connection succeeds.
2549 |   */
2550 | -bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, ConnectTrace& connectTrace, DisconnectedBlockTransactions &disconnectpool)
2551 | +bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, DisconnectedBlockTransactions &disconnectpool)
    MarcoFalke commented at 7:25 PM on March 23, 2020:

    style-nit:

    bool CChainState::ConnectTip(BlockValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const std::shared_ptr<const CBlock>& pblock, DisconnectedBlockTransactions& disconnectpool)
  31. MarcoFalke changes_requested
  32. MarcoFalke commented at 4:29 PM on March 27, 2020: member

    Looks like the wallet is using BlockConnected to determine when it is synced (BlockUntilSyncedToCurrentChain()). This is broken now.

  33. in src/validation.cpp:2788 in ab00be08e8 
    2783 | @@ -2826,14 +2784,13 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
2784 |          LimitValidationInterfaceQueue();
2785 |  
2786 |          {
2787 | -            LOCK2(cs_main, ::mempool.cs); // Lock transaction pool for at least as long as it takes for connectTrace to be consumed
2788 | +            // Do not unlock cs_main or mempool until we've made forward
2789 | +            // progress (with the exception of shutdown due to hardware issues,
    ariard commented at 1:22 AM on March 31, 2020:

    nit: define forward progress (chain tip is the valid most-work chain or most-work chain has been found invalid)

  34. in src/validation.cpp:2862 in ab00be08e8 
    2811 | @@ -2855,13 +2812,11 @@ bool CChainState::ActivateBestChain(BlockValidationState &state, const CChainPar
2812 |                      // Wipe cache, we may need another branch now.
2813 |                      pindexMostWork = nullptr;
2814 |                  }
2815 | +
2816 |                  pindexNewTip = m_chain.Tip();
2817 |  
2818 | -                for (const PerBlockConnectTrace& trace : connectTrace.GetBlocksConnected()) {
2819 | -                    assert(trace.pblock && trace.pindex);
2820 | -                    GetMainSignals().BlockConnected(trace.pblock, trace.pindex);
    ariard commented at 1:32 AM on March 31, 2020:

    Does it change anything for BlockConnected subscribers ? In case of block connections burst or reorgs, instead of receiving the diff at once after full validation, subscribers will now receive block connection after each individual validation in ConnectTip (assuming scheduler catch up). I think it's slightly better because discrepancy is shorter but likely a really marginal case.

  35. ariard approved
  36. ariard commented at 1:36 AM on March 31, 2020: member

    Code Review ACK ab00be0 @MarcoFalke can you describe your point further ? IIRC BlockUntilSyncedToCurrentChain waits until scheduler queue being drained before letting the wallet moving forward. From where we shot BlockConnected shouldn't change anything ?

  37. MarcoFalke commented at 3:17 AM on March 31, 2020: member

    @ariard If m_last_block_processed is the chain tip, it will not empty the scheduler queue, but return early, so it is going to miss tx updates in case of a reorg. Note that m_last_block_processed is set on blockConnected/Disconnected.

    See also my second bullet point here: #17562#pullrequestreview-379685371

  38. ariard commented at 1:52 AM on April 1, 2020: member

    https://doxygen.bitcoincore.org/class_c_validation_interface.html#a6de358ebc946cbd33ce73eef61993fe5 implies that mempool removal signals are ordered before block connection signals. After the changes in this pull request, this is no longer true. So an operation in a BlockConnected callback, that depends on an operation run in the TransactionRemovedFromMempool, might now fail.

    This PR doesn't inverse mempool removal signals and block connection signals as far as I understand. Mempool removal are triggered in removeUnchecked which is indirectly called via removeForBlock in ConnectTip before new call emplacement of BlockConnected. This order holds same for DisconnectTip.

    This order guarantee us than when wallet query the chain to know if it's behind (BlockUntilSyncedToCurrentChain) it's either wallet tip is equivalent to chain tip and there is not mempool update to wait for or wallet tip is lagging behind but catching up will only happen after TransactionsRemovedFromMempool has been drought out of the scheduler queue.

    I read again wallet code, as far as I can tell, transactionsRemovedFromMempool only tweak mempool flag for wallet transactions which is only used at broadcast but not for block connection.

    Or maybe I miss something, code isn't the clearest here ?

  39. MarcoFalke commented at 10:02 PM on April 13, 2020: member

    What about the ones in UpdateMempoolForReorg?

  40. jnewbery commented at 1:42 PM on April 17, 2020: member

    Thanks for pointing this out Marco. You're right that in a re-org, this changes the order of notification. If there's a one block diconnect/two block connect reorg, then before this PR:

    • Block A1 disconnected (txs added to disconnect pool)
      • BlockDisconnected notification for A1
    • Block A2 connected (block added to ConnectTrace)
      • TransactionRemovedFromMempool notifications for conflicted transactions
    • Block B2 connected (block added to ConnectTrace)
      • TransactionRemovedFromMempool notifications for conflicted transactions
    • UpdateMempoolForReorg called
      • TransactionRemovedFromMempool notifications for reorg-removed transactions
    • ActivateBestChainStep exits
      • BlockConnected notification for A2
      • BlockConnected notification for B2

    After this PR

    • Block A1 disconnected (txs added to disconnect pool)
      • BlockDisconnected notification for A1
    • Block A2 connected (block added to ConnectTrace)
      • TransactionRemovedFromMempool notifications for conflicted transactions
      • BlockConnected notification for A2
    • Block B2 connected (block added to ConnectTrace)
      • TransactionRemovedFromMempool notifications for conflicted transactions
      • BlockConnected notification for B2
    • UpdateMempoolForReorg called
      • TransactionRemovedFromMempool notifications for reorg-removed transactions
    • ActivateBestChainStep exits

    So at the point that BlockConnected is called for B2, the mempool is in an inconsistent state. As Marco points out, that should be ok since the validationinterface doesn't guarantee a consistent mempool when BlockConnected is fired. Listeners should use UpdatedBlockTip if they want that.

    However, I'm going to close this PR since it's difficult to review. It should be possible to bring all of the notification firing into ActivateBestChainStep() in a future PR, but to minimize review burden I'll open an alternative that doesn't remove ConnectTrace but instead just simplifies it and improves commenting.

  41. jnewbery closed this on Apr 17, 2020
  42. jnewbery commented at 2:40 PM on April 17, 2020: member

    Opened a PR to simplify ConnectTrace here: #18685

  43. DrahtBot locked this on Feb 15, 2022
Contributors
jnewberyDrahtBotariardjonatackMarcoFalkefjahr
Labels
Validation
Linked (view graph)
#18685 consensus: Simplify ConnectTrace
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: 2026-04-29 09:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me