document and clean up MaybeUpdateMempoolForReorg #23976
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
This was referenced Jan 5, 2022
|
ACK c633c41 |
maflcko
reviewed
Jan 5, 2022
|
Related #23897. |
hebasto
reviewed
Jan 5, 2022
There was a problem hiding this comment.
Concept ACK.
What about
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -348,11 +348,11 @@ void CChainState::MaybeUpdateMempoolForReorg(
AssertLockHeld(m_mempool->cs);
AssertLockHeld(::cs_main);
const CTransaction& tx = it->GetTx();
+ if (!CheckFinalTx(m_chain.Tip(), tx, flags)) return true;
LockPoints lp = it->GetLockPoints();
const bool validLP{TestLockPointValidity(m_chain, lp)};
CCoinsViewMemPool view_mempool(&CoinsTip(), *m_mempool);
// The transaction must be final.
- if (!CheckFinalTx(m_chain.Tip(), tx, flags)) return true;
// Note if CheckSequenceLocks fails the LockPoints may still be invalid
// So it's critical that we remove the tx and not depend on the LockPoints.
if (!CheckSequenceLocks(m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) {?
instagibbs
reviewed
Jan 10, 2022
This was referenced Jan 11, 2022
|
Sorry for the delay! I've addressed @hebasto and @instagibbs comments and grouped in @hebasto's commit from #23958. |
hebasto
reviewed
Jan 18, 2022
Co-authored-by: John Newbery <john@johnnewbery.com>
No behavior change. This code was introduced in 5add7a7 before we required C++11, which is why the struct was needed. As we are now using more modern C++ and this is the only place where lockpoints are updated for mempool entries, it is more idiomatic to call `modify` with a lambda. Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Oops! Good catch, fixed. |
maflcko
approved these changes
Jan 19, 2022
| @@ -372,18 +388,16 @@ void CChainState::MaybeUpdateMempoolForReorg( | |||
| assert(!coin.IsSpent()); | |||
| const auto mempool_spend_height{m_chain.Tip()->nHeight + 1}; | |||
| if (coin.IsSpent() || (coin.IsCoinBase() && mempool_spend_height - coin.nHeight < COINBASE_MATURITY)) { | |||
There was a problem hiding this comment.
nit: any reason to check for coin.IsSpent(), given the assertion?
maflcko
approved these changes
Jan 19, 2022
There was a problem hiding this comment.
Approach ACK e177fca 😶
Did not review the last commit, but given the assert added in b4adc5a it should be correct.
Show signature
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
Approach ACK e177fcab3831b6d259da5164cabedcc9e78f6957 😶
Did not review the last commit, but given the assert added in b4adc5ad6769e4a5a6179dfff271cd4c9dc47a5b it should be correct.
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUj/Ugv+N4lEh7A93jSO5imukN2scMEs6r7j7KNiTudrany2XPrSbTvJQgl+q46K
j5UPWvQCzJ0M+NpenYE0Eiefl51eZGRo+4rT06GlQJ/jNyZkutbVmbbNvspM19gg
lsnpx7pfaLR9MFAmIZQ3xz2y5zu2adewPtFrTAfKbxfkZbkR17+EayICGvXRked9
u/UGf/F/XmA/Yopnfn4jAHpPUlvvgKFxOaRFI76302IDY6D9XgzTAg2cjLBlUuy9
3KCf5UGPJ2fVYFcpf4OZdsGi+vzo17+UGOK0dcYb/wLqfr2VH/mZPL5o/jo2l3d4
EcGvDAzYRrM3tvRp9pFDO0O27avyvMw106RI/u1/TyqKX4apcblX954bAugIBXuN
ZgAed34pqaHeRqm9R1WdY1O270pehbUtv+NKWu6awEZ+j79vwF8UVwOModkepwbd
Fp0c+f4aEAzHLEEdpMR2l+XUrF97ZviaVNGXX06bfuIpSsw7MpR3K98SMmLxVyn9
CVsQAgjV
=YL6U
-----END PGP SIGNATURE-----
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jan 20, 2022
maflcko
pushed a commit
to bitcoin-core/gui
that referenced
this pull request
Jan 24, 2022
… row fa2bcc4 Run coin.IsSpent only once in a row (MarcoFalke) Pull request description: Follow-up to commit 64e4963 and bitcoin/bitcoin#23976 (comment) ACKs for top commit: glozow: utACK fa2bcc4, agree the assertion is sufficient theStack: Code-review ACK fa2bcc4 w0xlt: crACK bitcoin/bitcoin@fa2bcc4 shaavan: Code Review ACK fa2bcc4 brunoerg: crACK fa2bcc4 Tree-SHA512: 3be9d6b313bf6bb835f031826c81777b4659118d839001d084e72462391cb64ba81d06a5e07fd21fcfb709a71b08892b23212a98604ce8481da489476b72f072
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jan 28, 2022
fa2bcc4 Run coin.IsSpent only once in a row (MarcoFalke) Pull request description: Follow-up to commit 64e4963 and bitcoin#23976 (comment) ACKs for top commit: glozow: utACK fa2bcc4, agree the assertion is sufficient theStack: Code-review ACK fa2bcc4 w0xlt: crACK bitcoin@fa2bcc4 shaavan: Code Review ACK fa2bcc4 brunoerg: crACK fa2bcc4 Tree-SHA512: 3be9d6b313bf6bb835f031826c81777b4659118d839001d084e72462391cb64ba81d06a5e07fd21fcfb709a71b08892b23212a98604ce8481da489476b72f072
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 24, 2022
Summary: Co-authored-by: John Newbery <john@johnnewbery.com> This is a partial backport of [[bitcoin/bitcoin#23976 | core#23976]] bitcoin/bitcoin@c7cd98c Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12587
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Nov 24, 2022
Summary: No behavior change. This code was introduced in 5add7a7 before we required C++11, which is why the struct was needed. As we are now using more modern C++ and this is the only place where lockpoints are updated for mempool entries, it is more idiomatic to call `modify` with a lambda. Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> This concludes backport of [[bitcoin/bitcoin#23976 | core#23976]] bitcoin/bitcoin@e177fca Depends on D12587 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12588
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
followup to #23683, addressing #23683 (comment)