[Bundle 2/n] Prune g_chainman usage in mempool-related validation functions #20750

pull dongcarl wants to merge 20 commits into bitcoin:master from dongcarl:2020-09-reduce-validation-mempool-ccsactiveglobal-usage changing 14 files +114 −74
  1. dongcarl commented at 7:35 pm on December 22, 2020: member

    Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)

    Note to reviewers:

    1. This bundle may apparently introduce usage of g_chainman or ::Chain(state|)Active() globals, but these are resolved later on in the overall PR. Commits of overall PR
    2. There may be seemingly obvious local references to ChainstateManager or other validation objects which are not being used in callers of the current function in question, this is done intentionally to keep each commit centered around one function/method to ease review and to make the overall change systematic. We don’t assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. Commits of overall PR
    3. When changing a function/method that has many callers (e.g. LookupBlockIndex with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
      1. Add new_function, make old_function a wrapper of new_function, divert all calls to old_function to new_function in the local module only
      2. Scripted-diff to divert all calls to old_function to new_function in the rest of the codebase
      3. Remove old_function
  2. dongcarl added the label Refactoring on Dec 22, 2020
  3. DrahtBot commented at 8:40 pm on December 22, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #21148 (Split orphan handling from net_processing into txorphanage by ajtowns)
    • #21146 (validation/coins: limit MemPoolAccept access to coins cache + make it responsible for uncaching by glozow)
    • #21003 (test: Move MakeNoLogFileContext to libtest_util, and use it in bench by MarcoFalke)
    • #20833 (rpc/validation: enable packages through testmempoolaccept by glozow)
    • #19259 (fuzz: Add fuzzing harness for LoadMempool(…) and DumpMempool(…) by practicalswift)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  4. DrahtBot added the label Needs rebase on Jan 7, 2021
  5. DrahtBot commented at 11:46 am on January 13, 2021: member

    🕵️ @sipa @fjahr have been requested to review this pull request as specified in the REVIEWERS file.

  6. dongcarl force-pushed on Jan 27, 2021
  7. DrahtBot removed the label Needs rebase on Jan 27, 2021
  8. jnewbery commented at 12:17 pm on February 1, 2021: member
    #20749 is merged. Rebase please :slightly_smiling_face:
  9. DrahtBot added the label Needs rebase on Feb 1, 2021
  10. dongcarl force-pushed on Feb 1, 2021
  11. dongcarl commented at 4:57 pm on February 1, 2021: member

    Pushed 5803d30babf19a86828b1d967b0d38ee8c6551c4 -> 8d435f277f44dbd6807c809c34411b6f1f85b4bc

    • Rebased over master since “#20749 | [Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndex” is now merged!
  12. DrahtBot removed the label Needs rebase on Feb 1, 2021
  13. in src/validation.cpp:205 in 8d435f277f outdated 
    201@@ -202,9 +202,10 @@ static FILE* OpenUndoFile(const FlatFilePos &pos, bool fReadOnly = false);
202 static FlatFileSeq BlockFileSeq();
203 static FlatFileSeq UndoFileSeq();
204 
205-bool CheckFinalTx(const CTransaction &tx, int flags)
206+bool CheckFinalTx(CBlockIndex* active_chain_tip, const CTransaction &tx, int flags)
    ariard commented at 9:27 pm on February 1, 2021:
    I think you can make this a const ?
    dongcarl commented at 2:48 am on February 2, 2021:
    Done! See: e5198eff4b80d0d39d1cd9b3da317b25026c436d
  14. in src/test/validation_block_tests.cpp:289 in 79e346d1d7 outdated 
    285@@ -286,7 +286,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
286             TxValidationState state;
287             std::list<CTransactionRef> plTxnReplaced;
288             for (const auto& tx : txs) {
289-                BOOST_REQUIRE(AcceptToMemoryPool(
290+                BOOST_REQUIRE(AcceptToMemoryPool(::ChainstateActive(),
    ariard commented at 10:06 pm on February 1, 2021:
    whitespace?
    dongcarl commented at 2:36 am on February 2, 2021:
    This is fixed in the subsequent commit!
  15. in src/txmempool.cpp:359 in 79e346d1d7 outdated 
    355@@ -356,7 +356,7 @@ void CTxMemPool::AddTransactionsUpdated(unsigned int n)
356 void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAncestors, bool validFeeEstimate)
357 {
358     // Add to memory pool without checking anything.
359-    // Used by AcceptToMemoryPool(), which DOES do
360+    // Used by AcceptToMemoryPool(::ChainstateActive(), ), which DOES do
    ariard commented at 10:08 pm on February 1, 2021:

    I think the scripted-diff is too much expansive here.

    EDIT: note for reviewers, fixed in next commit.

    dongcarl commented at 2:37 am on February 2, 2021:
    Yeah I should have used a clang-tidy script for this… Oh well, perhaps we can set that up in the future.
    MarcoFalke commented at 6:58 pm on February 19, 2021:
    nit: maybe exclude this file from the scripted diff? Also nit: git comes with an exclude syntax for files, so you don’t have to use grep -v
    dongcarl commented at 9:43 pm on February 19, 2021:
    Yeah the git syntax was confusing so I wanted to just use grep since it’s more commonly used 😬. I will make this change if there are other substantial changes that requires a push!
  16. ariard commented at 10:19 pm on February 1, 2021: member

    Approach ACK, did a first parse on each commit.

    Side-question, what’s your heuristic to divide your bundles ?

  17. dongcarl force-pushed on Feb 2, 2021
  18. dongcarl commented at 2:47 am on February 2, 2021: member

    Pushed 8d435f277f44dbd6807c809c34411b6f1f85b4bc -> 64db9d61f4c23acb8b68b1a778db02aed0f582f1

  19. glozow commented at 7:00 pm on February 2, 2021: member
    Concept ACK, planning to review more closely soon. Would it ever be the case that we call ATMP(chainstate, pool...) where pool != chainstate.m_mempool? If not, it seems like you could take pool out of the function signature(s) as well. (oh jk 😳 it’s protected)
  20. dongcarl commented at 8:13 pm on February 2, 2021: member

    Side-question, what’s your heuristic to divide your bundles ?

    It’s mostly bundles of functions which call each other, not too scientific, but works!

  21. in src/validation.cpp:473 in dd7e989b0a outdated 
    463         m_limit_ancestors(gArgs.GetArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT)),
464         m_limit_ancestor_size(gArgs.GetArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT)*1000),
465         m_limit_descendants(gArgs.GetArg("-limitdescendantcount", DEFAULT_DESCENDANT_LIMIT)),
466-        m_limit_descendant_size(gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000) {}
467+        m_limit_descendant_size(gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000) {
468+        assert(std::addressof(::ChainstateActive()) == std::addressof(m_active_chainstate));
    ryanofsky commented at 8:50 pm on February 2, 2021:

    In commit “validation: Add chainstate member to MemPoolAccept” (dd7e989b0a4ad6949ccb70d13405f7af3f05b724)

    Assert might be more meaningful if it were added all the places the new m_active_chainstate member is used

    dongcarl commented at 7:59 pm on February 8, 2021:
    Agreed! Done in: 76a4aa60dbc8e142a72c84cf35ebc04c7654866f
  22. ryanofsky approved
  23. ryanofsky commented at 9:00 pm on February 2, 2021: member
    Code review ACK 64db9d61f4c23acb8b68b1a778db02aed0f582f1
  24. in src/validation.h:231 in 64db9d61f4 outdated 
    227@@ -228,7 +228,7 @@ bool TestLockPointValidity(const LockPoints* lp) EXCLUSIVE_LOCKS_REQUIRED(cs_mai
228  *
229  * See consensus/consensus.h for flag definitions.
230  */
231-bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp = nullptr, bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs);
232+bool CheckSequenceLocks(CChainState& active_chainstate, const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp = nullptr, bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs);
    jnewbery commented at 1:42 pm on February 4, 2021:

    I find these 200+ character lines very difficult to parse. It’d be nice if PRs didn’t make them even longer and more unreadable. Consider something like:

    0bool CheckSequenceLocks(CChainState& active_chainstate, const CTxMemPool& pool,
1                        const CTransaction& tx, int flags, LockPoints* lp = nullptr,
2                        bool useExistingLockPoints = false)
3    EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs);

    (same for the function definition in validation.cpp)

    dongcarl commented at 7:59 pm on February 8, 2021:
    Done in: bf2927643162c650f2a54dd33a83eccaa67ac50c, added a not to myself to review the future commits for this!
  25. in src/validation.h:187 in 64db9d61f4 outdated 
    183@@ -184,7 +184,7 @@ void PruneBlockFilesManual(int nManualPruneHeight);
184 /** (try to) add transaction to memory pool
185  * plTxnReplaced will be appended to with all transactions replaced from mempool
186  * @param[out] fee_out optional argument to return tx fee to the caller **/
187-bool AcceptToMemoryPool(CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx,
188+bool AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx,
    jnewbery commented at 1:47 pm on February 4, 2021:
    Again, this change makes it harder to read this function declaration.
    dongcarl commented at 8:00 pm on February 8, 2021:
    Done in: 2eceb88e52e06e5d58c864b67ff65bc91749eeb1, added a not to myself to review the future commits for this!
  26. in src/txmempool.cpp:502 in 64db9d61f4 outdated 
    498@@ -499,16 +499,17 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso
499         RemoveStaged(setAllRemoves, false, reason);
500 }
501 
502-void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags)
503+void CTxMemPool::removeForReorg(CChainState& active_chainstate, const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags)
    jnewbery commented at 1:52 pm on February 4, 2021:
    There’s no need to pass in a CCoinsViewCache * or nMemPoolHeight here any more. You can just get them from the CChainState&.
    dongcarl commented at 8:00 pm on February 8, 2021:
    Done in: 6dd22bc4b052f42cb9d102e940a4196d62c975fb Much cleaner!
  27. jnewbery commented at 1:59 pm on February 4, 2021: member

    Code review ACK 64db9d61f4c23acb8b68b1a778db02aed0f582f1

    Would it ever be the case that we call ATMP(chainstate, pool…) where pool != chainstate.m_mempool? If not, it seems like you could take pool out of the function signature(s) as well.

    I agree with this. Anywhere that you’re passing a CChainState&, you can avoid also passing a CTxMemPool& (after making CChainState.m_mempool public). It wouldn’t make sense to call one of these functions with a different mempool from the one associated with the chainstate (and besides, we only ever have one mempool).

  28. glozow commented at 0:31 am on February 6, 2021: member

    I have a question - when is it necessary to pass the entire CChainState? For most of these functions, it makes perfect sense to me (and I think args should be removed when they can be retrieved from the chainstate), but in others, it seems like the function could just use a specific member of chainstate because their responsibility is narrower.

    For example, in e83c8538ec022fa29bed2894471cdee8a974f66f, CheckInputsFromMempoolAndCache() just looks at the coins in view and checks that they’re consistent with mempool and current CoinsTip(), then calls CheckInputScripts() (which should be stateless). So it really only needs an extra CCoinsViewCache& from chainstate.CoinsTip().

    In 76e0811629cfe94a19a2ee52aff51bcba073b55c, it seems to me that CheckSequenceLocks() only really needs the CBlockIndex* chain tip, similarly to CheckFinalTx()?

  29. ariard commented at 12:32 pm on February 8, 2021: member
    Code Review ACK 64db9d6
  30. dongcarl force-pushed on Feb 8, 2021
  31. dongcarl commented at 8:04 pm on February 8, 2021: member

    @glozow

    I have a question - when is it necessary to pass the entire CChainState? For most of these functions, it makes perfect sense to me (and I think args should be removed when they can be retrieved from the chainstate), but in others, it seems like the function could just use a specific member of chainstate because their responsibility is narrower.

    You’re right! I think most of this is vestigial stuff from when I was trying to keep things safe and keep the code as close to existing code as possible. However, with enough asserts, we can be more confident that these changes are a-okay.

    For example, in e83c853, CheckInputsFromMempoolAndCache() just looks at the coins in view and checks that they’re consistent with mempool and current CoinsTip(), then calls CheckInputScripts() (which should be stateless). So it really only needs an extra CCoinsViewCache& from chainstate.CoinsTip().

    Good catch! Fixed in: f6de0ae51e5b3b824e7eef6b988904b67cd3dab0

    In 76e0811, it seems to me that CheckSequenceLocks() only really needs the CBlockIndex* chain tip, similarly to CheckFinalTx()?

    I think CheckSequenceLocks() needs both active_chainstate.m_chain.Tip() and active_chainstate.CoinsTip()? I may be missing something here.

  32. in src/validation.cpp:338 in 2eceb88e52 outdated 
    334@@ -327,7 +335,7 @@ bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flag
335 // Returns the script flags which should be checked for a given block
336 static unsigned int GetBlockScriptFlags(const CBlockIndex* pindex, const Consensus::Params& chainparams);
337 
338-static void LimitMempoolSize(CTxMemPool& pool, size_t limit, std::chrono::seconds age)
339+static void LimitMempoolSize(CTxMemPool& pool, CChainState& chainstate, size_t limit, std::chrono::seconds age)
    glozow commented at 3:39 pm on February 9, 2021:
    I thiiiiink this only needs the CCoinsViewCache CoinsTip?
  33. in src/validation.cpp:381 in 2eceb88e52 outdated 
    375@@ -366,10 +376,11 @@ static bool IsCurrentForFeeEstimation() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
376  * and instead just erase from the mempool as needed.
377  */
378 
379-static void UpdateMempoolForReorg(CTxMemPool& mempool, DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, mempool.cs)
380+static void UpdateMempoolForReorg(CChainState& active_chainstate, CTxMemPool& mempool, DisconnectedBlockTransactions& disconnectpool, bool fAddToMempool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, mempool.cs)
    glozow commented at 3:40 pm on February 9, 2021:
    mempool not needed if you can get it from active_chainstate
  34. in src/validation.cpp:5025 in 2eceb88e52 outdated 
    5024@@ -4991,7 +5025,7 @@ int VersionBitsTipStateSinceHeight(const Consensus::Params& params, Consensus::D
5025 
5026 static const uint64_t MEMPOOL_DUMP_VERSION = 1;
5027 
5028-bool LoadMempool(CTxMemPool& pool)
5029+bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate)
    glozow commented at 3:41 pm on February 9, 2021:
    same, pool not needed if you can get it from active_chainstate
  35. in src/validation.h:269 in 2eceb88e52 outdated 
    232@@ -228,7 +233,12 @@ bool TestLockPointValidity(const LockPoints* lp) EXCLUSIVE_LOCKS_REQUIRED(cs_mai
233  *
234  * See consensus/consensus.h for flag definitions.
235  */
236-bool CheckSequenceLocks(const CTxMemPool& pool, const CTransaction& tx, int flags, LockPoints* lp = nullptr, bool useExistingLockPoints = false) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, pool.cs);
237+bool CheckSequenceLocks(CChainState& active_chainstate,
    glozow commented at 3:41 pm on February 9, 2021:
    same, pool not needed if you can get it from active_chainstate
  36. in src/validation.h:187 in 2eceb88e52 outdated 
    183@@ -184,9 +184,14 @@ void PruneBlockFilesManual(int nManualPruneHeight);
184 /** (try to) add transaction to memory pool
185  * plTxnReplaced will be appended to with all transactions replaced from mempool
186  * @param[out] fee_out optional argument to return tx fee to the caller **/
187-bool AcceptToMemoryPool(CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx,
188+bool AcceptToMemoryPool(CChainState& active_chainstate,
    glozow commented at 3:41 pm on February 9, 2021:
    same, pool not needed if you can get it from active_chainstate
  37. in src/validation.cpp:353 in 2eceb88e52 outdated 
    350-        ::ChainstateActive().CoinsTip().Uncache(removed);
351+        chainstate.CoinsTip().Uncache(removed);
352 }
353 
354-static bool IsCurrentForFeeEstimation() EXCLUSIVE_LOCKS_REQUIRED(cs_main)
355+static bool IsCurrentForFeeEstimation(CChainState& chainstate) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
    glozow commented at 3:51 pm on February 9, 2021:
    nit: not sure why this is chainstate and not active_chainstate like the others?
  38. glozow commented at 3:56 pm on February 9, 2021: member

    code review ACK https://github.com/bitcoin/bitcoin/pull/20750/commits/2eceb88e52e06e5d58c864b67ff65bc91749eeb1, Cirrus timeout should be fixed with #21117

    Not trying to be annoying but leaving a trail of “don’t need mempool if you have chainstate” comments in the case it’s made a public member here / in the future.

    I think CheckSequenceLocks() needs both active_chainstate.m_chain.Tip() and active_chainstate.CoinsTip()? I may be missing something here.

    Oops @dongcarl you’re right 🙈

  39. DrahtBot added the label Needs rebase on Feb 11, 2021
  40. ryanofsky approved
  41. ryanofsky commented at 8:48 pm on February 11, 2021: member
    Code review ACK 2eceb88e52e06e5d58c864b67ff65bc91749eeb1. Changes since last review: dropping removeForReorg & CheckInputsFromMempoolAndCache arguments, adding m_active_chainstate asserts, adding whitespace commits
  42. ariard commented at 1:56 pm on February 12, 2021: member
    Code Review ACK 2eceb88, code changes are adding new asserts in few locations and dropping CChainState /CCoinsViewCache args when only one was useful. Also dropping passing nMemPoolHeight through removeForReorg.
  43. dongcarl force-pushed on Feb 16, 2021
  44. dongcarl commented at 6:43 pm on February 16, 2021: member

    Pushed 2eceb88e52e06e5d58c864b67ff65bc91749eeb1 -> 15f0042cb77c0dc4e4f137172d6e871dd2bc55a9

  45. glozow commented at 8:07 pm on February 16, 2021: member

    reACK https://github.com/bitcoin/bitcoin/commit/15f0042cb77c0dc4e4f137172d6e871dd2bc55a9

    Only changes are renaming to active_chainstate in IsCurrentForFeeEstimation and using coins_cache in LimitMempoolSize

  46. DrahtBot removed the label Needs rebase on Feb 16, 2021
  47. ryanofsky approved
  48. ryanofsky commented at 0:42 am on February 18, 2021: member
    Code review ACK 15f0042cb77c0dc4e4f137172d6e871dd2bc55a9. A lot of rebase conflicts and LimitMempoolSize param change since last review.
  49. MarcoFalke commented at 9:27 am on February 18, 2021: member
    Needs rebase
  50. laanwj commented at 11:58 am on February 18, 2021: member

    Needs rebase

    It does, git manages to merge but currently the build fails with

    0/…/bitcoin/src/test/util/setup_common.cpp:305:44: error: no matching function for call to 'AcceptToMemoryPool'
1        const MempoolAcceptResult result = AcceptToMemoryPool(*m_node.mempool.get(), MakeTransactionRef(mempool_txn), /* bypass_limits */ false);
2                                           ^~~~~~~~~~~~~~~~~~
3/…/bitcoin/src/validation.h:226:21: note: candidate function not viable: requires at least 4 arguments, but 3 were provided
4MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
5                    ^
61 error generated.
  51. validation: Pass in coins cache to ::LimitMempoolSize d1f932b0b0
  52. validation: Pass in chainstate to IsCurrentForFeeEstimation 73a6d2b7be
  53. validation: Pass in coins tip to CheckInputsFromMempoolAndCache 252b489c9f
  54. validation: Pass in chain tip to ::CheckFinalTx d015eaa550
  55. scripted-diff: Invoke ::CheckFinalTx with chain tip 
    -BEGIN VERIFY SCRIPT-
find_regex='\bCheckFinalTx\(' \
    && git grep -l -E "$find_regex" -- src \
        | grep -v '^src/validation\.\(cpp\|h\)$' \
        | xargs sed -i -E 's@'"$find_regex"'@\0::ChainActive().Tip(), @g'
-END VERIFY SCRIPT-
    7031cf89db
  56. validation: Remove old CheckFinalTx w/o chain tip param 577b774d0c
  57. validation: Pass in chainstate to ::CheckSequenceLocks 4c15942b79
  58. validation: Add chainstate member to MemPoolAccept d8a816329c
  59. validation: Pass in chainstate to AcceptToMemoryPoolWithTime 3a205c43dc
  60. validation: Pass in chainstate to ::LoadMempool d0da7ea57a
  61. validation: Pass in chainstate to ::AcceptToMemoryPool 229bc37b5f
  62. scripted-diff: Invoke ::AcceptToMemoryPool with chainstate 
    -BEGIN VERIFY SCRIPT-
find_regex='\bAcceptToMemoryPool\(' \
    && git grep -l -E "$find_regex" -- src \
        | grep -v '^src/validation\.\(cpp\|h\)$' \
        | xargs sed -i -E 's@'"$find_regex"'@\0::ChainstateActive(), @g'
-END VERIFY SCRIPT-
    3704433c4f
  63. validation: Remove old AcceptToMemoryPool w/o chainstate param 417dafc1ee
  64. tree-wide: Fix erroneous AcceptToMemoryPool replacements 120aaba9ac
  65. validation: Pass in chain to ::TestLockPointValidity 71734c65dc
  66. validation: Pass in chainstate to CTxMemPool::removeForReorg 
    Several other parameters are now redundant since they can be safely
obtained from the chainstate given that ::cs_main is locked. These are
now removed.
    7142018812
  67. validation: Pass in chainstate to UpdateMempoolForReorg 0a9a24d8c7
  68. validation: Use *this in CChainState::LoadMempool 8c824819c8
  69. style-only: Make CheckSequenceLock signature readable 8f5c100064
  70. style-only: Make AcceptToMemoryPool signature readable e8ae1db864
  71. dongcarl force-pushed on Feb 18, 2021
  72. dongcarl commented at 7:57 pm on February 18, 2021: member

    Pushed 15f0042cb77c0dc4e4f137172d6e871dd2bc55a9 -> e8ae1db864b09a47c736631e6cd3f5ec17929850

  73. in src/validation.cpp:212 in d015eaa550 outdated 
    204@@ -205,8 +205,14 @@ static FlatFileSeq BlockFileSeq();
205 static FlatFileSeq UndoFileSeq();
206 
207 bool CheckFinalTx(const CTransaction &tx, int flags)
208+{
209+    return CheckFinalTx(::ChainActive().Tip(), tx, flags);
210+}
211+
212+bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, int flags)
    MarcoFalke commented at 6:46 pm on February 19, 2021:
    nit in d015eaa550027a387cd548cf0bcfa1a4c31a3374: For new code it would be good to use the new style (& is left-attaching). Also, what about passing a const reference instead of a pointer? I know this is only theoretical, but previously if the tip was nullptr, then Height() evaluated to -1, now it evaluates to UB. Also, in the comment in this function, you’ll need to replace ::ChainActive().Height()+1 as well? Finally, the next two commits could just be squashed into this one?
    dongcarl commented at 9:42 pm on February 19, 2021:
    Right, this makes sense. I’m going to put this nit off until the next bundle so that I don’t invalidate ACKs if that’s alright with you! I added it as a “Note to self” in the next bundle.
  74. MarcoFalke commented at 7:13 pm on February 19, 2021: member

    ACK e8ae1db864b09a47c736631e6cd3f5ec17929850 📣

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e8ae1db864b09a47c736631e6cd3f5ec17929850 📣
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgMzwwAsEQvRsGjLFTPiwkdMs4DZSXcqw2e+bK/3idvLQRv6sJ7vqrS5W3UIqH4
 8POgM2H1pz6rbusNlvyXaeNIG/C+RGniPVKf0pBwPrK+Wo/PRpTMMTPyl9cUh3hfB
 9Xb4HhOCux29lfYNQNFUVq7+O9k2hj9634Q5QCNZkE988rIiGZt6hYc38RIMcSeY7
10LU6Pbbc7P7eembZfocL2nVonunPvZij0mLmILc3JWX/jt4C3IqJ6uiQP56ALb08d
11gUMuyuQibGz6ptK9S5XeLr2/YyTYt7Z5UNCGZtJyxshFA5MdAxcDhMKQZhs2dW1K
12w2jtnuWVWFfHgaU1OvsTjaKmLP8UsCwD4QxJzIgaUgFX//5eWBTN60nyvLVp5Ara
130KndogF/nMxu1KXgVcD7R+LuJiSOJivBX8So9l9wtWwTZdBhb8gybTani25Ef2I4
14oB0BSi0GsJlhyvSKBjkvxpzlnf1ofDJ9b6Mid9Z/PgUI50rIO2JixDU5WgXoBvRd
15ikxxBzeX
16=XgwZ
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 4c3078abec10b46f834a4805961a34857896683a0701dfcccf98cc987f6a924e -

  75. glozow commented at 8:56 pm on February 19, 2021: member
    reACK https://github.com/bitcoin/bitcoin/commit/e8ae1db864b09a47c736631e6cd3f5ec17929850 via git range-diff 15f0042...e8ae1db, only change is fixing ATMP call from conflict
  76. MarcoFalke merged this on Feb 20, 2021
  77. MarcoFalke closed this on Feb 20, 2021
  78. sidhujag referenced this in commit 3bdd88cc15 on Feb 20, 2021
  79. dongcarl referenced this in commit 9da106be4d on Feb 22, 2021
  80. laanwj referenced this in commit 702cfc8c53 on Mar 4, 2021
  81. theStack referenced this in commit 9be318c5e8 on Mar 5, 2021
  82. Fabcien referenced this in commit a26c5e7a96 on Mar 16, 2022
  83. Fabcien referenced this in commit 9cec0a4c8c on Mar 16, 2022
  84. Fabcien referenced this in commit 98315e93d5 on Mar 16, 2022
  85. Fabcien referenced this in commit 3e47401a58 on Mar 17, 2022
  86. Fabcien referenced this in commit 21679a6f0b on Mar 17, 2022
  87. Fabcien referenced this in commit 2dfdbfebb9 on Mar 17, 2022
  88. Fabcien referenced this in commit 4afce42c41 on Mar 17, 2022
  89. Fabcien referenced this in commit e01f4636ab on Mar 17, 2022
  90. Fabcien referenced this in commit a53d338759 on Mar 17, 2022
  91. Fabcien referenced this in commit 4e68502c84 on Mar 17, 2022
  92. Fabcien referenced this in commit 1e0222efb3 on Mar 17, 2022
  93. Fabcien referenced this in commit 110cc42f6c on Mar 17, 2022
  94. Fabcien referenced this in commit 184be66e5c on Mar 17, 2022
  95. DrahtBot locked this on Aug 16, 2022


dongcarl DrahtBot jnewbery ariard MarcoFalke glozow ryanofsky laanwj

Labels
Refactoring

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-12-18 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me