refactor: CheckFinalTx pass by reference instead of pointer #22282

pull klementtan wants to merge 1 commits into bitcoin:master from klementtan:CheckFinalTx_ptr_to_ref changing 5 files +12 −13

klementtan commented at 9:13 AM on June 19, 2021: contributor

Rationale This PR fixes TODO from #21055

https://github.com/bitcoin/bitcoin/blob/965e93743454112c0c3c66bf24852f63ee07b862/src/validation.cpp#L183

#20750 (review) for additional information.

Changes to CheckFinalTx: active_chain_tip to be passed by reference instead of pointer and shift & of tx to be left-attaching
refactor: CheckFinalTx pass by reference instead of pointer 10caa3e4b1
DrahtBot added the label Mempool on Jun 19, 2021
DrahtBot added the label Refactoring on Jun 19, 2021
DrahtBot added the label Validation on Jun 19, 2021
klementtan renamed this:
~~refactor: CheckFinalTx pass by reference instead of ptr~~
refactor: CheckFinalTx pass by reference instead of pointer
on Jun 19, 2021
MarcoFalke removed the label Mempool on Jun 19, 2021
MarcoFalke removed the label Validation on Jun 19, 2021
theStack approved
theStack commented at 1:04 PM on June 19, 2021: member

Code-review ACK 10caa3e4b167dae26d52838ad19f8c137c9c0a14 🥥
benthecarman commented at 6:41 AM on June 20, 2021: contributor

ack 10caa3e4b167dae26d52838ad19f8c137c9c0a14
practicalswift commented at 11:24 AM on June 20, 2021: contributor

Concept ACK

Candidates for similar treatment can be found using the git grep commands in #19062 :)
klementtan commented at 1:03 PM on June 20, 2021: contributor

Candidates for similar treatment can be found using the git grep commands in #19062 :)

Thanks for informing me be about this. Added a comment on the issue to reference this PR. However, I think I will keep the scope of this PR to CheckFianlTx to make it easy to review.
DrahtBot commented at 9:49 PM on August 10, 2021: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #22677 (cut the validation <-> txmempool circular dependency 2/2 by glozow)
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.

in src/validation.cpp:183 in 10caa3e4b1

 176 | @@ -177,10 +177,9 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
 177 |                         std::vector<CScriptCheck>* pvChecks = nullptr)
 178 |                         EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 179 |  
 180 | -bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, int flags)
 181 | +bool CheckFinalTx(const CBlockIndex& active_chain_tip, const CTransaction& tx, int flags)
 182 |  {
 183 |      AssertLockHeld(cs_main);
 184 | -    assert(active_chain_tip); // TODO: Make active_chain_tip a reference

MarcoFalke commented at 5:45 PM on December 1, 2021:

This removes an assertion and turns a (theoretical) crash into UB

DrahtBot commented at 6:39 PM on December 1, 2021: member



🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
DrahtBot added the label Needs rebase on Dec 1, 2021
klementtan commented at 5:47 PM on December 12, 2021: contributor

Closing this PR as this refactor will remove an assertion.
klementtan closed this on Dec 12, 2021
MarcoFalke commented at 8:01 AM on December 13, 2021: member

As long as you preserve the assertion, it should be ok.
DrahtBot locked this on Dec 13, 2022

Contributors

Labels

Refactoring Needs rebase

Linked (view graph)

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

refactor: CheckFinalTx pass by reference instead of pointer #22282

Conflicts