Add `ChainstateManager::ProcessTransaction` #23173

jnewbery · 2021-10-04T13:23:59Z

Similarly to how #18698 added ProcessNewBlock() and ProcessNewBlockHeaders() methods to the ChainstateManager class, this PR adds a new ProcessTransaction() method. Code outside validation no longer calls AcceptToMemoryPool() directly, but calls through the higher-level ProcessTransaction() method. Advantages:

The interface is simplified. Calling code no longer needs to know about the active chainstate or mempool object, since AcceptToMemoryPool() can only ever be called for the active chainstate, and that chainstate knows which mempool it's using. We can also remove the bypass_limits argument, since that can only be used internally in validation.
responsibility for calling CTxMemPool::check() is removed from the callers, and run automatically by ChainstateManager every time ProcessTransaction() is called.

maflcko

Concept ACK

src/test/util/setup_common.cpp

DrahtBot · 2021-10-04T14:22:59Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21527 (net_processing: lock clean up by ajtowns)
#21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

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.

jamesob · 2021-10-04T14:47:01Z

Concept ACK, looks like a good change. Will review soon.

jnewbery · 2021-10-04T16:09:04Z

Resolved @MarcoFalke's review comment and added a missing lock annotation.

glozow

Concept ACK

practicalswift · 2021-10-05T10:21:44Z

Concept ACK

jnewbery · 2021-10-07T09:51:12Z

Fixed up an error string and rebased on master to (hopefully) pick up fixes for unrelated CI failures. The fuzz test will still fail, so I'll try to fix that next.

jnewbery · 2021-10-07T14:00:28Z

I've reverted the change that made AcceptToMemoryPool() static and left it as dynamically linkable for use by the mempool fuzz test (which was operating on a separate CChainState and CTxMemPool).

glozow

Did a first pass-through. I like that this re-delegates the responsibility of calling mempool.check() from net_processing to chainstate manager, and see it as the main purpose of this refactoring.

src/validation.cpp

src/validation.h

promag

Concept ACK.

Light code review ACK 1d20d43.

jnewbery · 2021-10-19T17:14:17Z

Thanks for the reviews @glozow and @promag. I've addressed all review comments.

glozow

code review ACK 3bb34f0

A few small suggestions if you retouch. The comment touchups sprinkled throughout the commits make the diffs a little noisy.

src/validation.h

src/validation.cpp

src/validation.h

AcceptToMemoryPool() is called for transactions with fees above minRelayTxFee and with the mempool not full, so setting bypass_limits to true or false has no impact on the test. The only way that changing bypass_limits from true to false could change the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY). Since all the ATMP calls in this test result in VALID both before and after this change, there is no change in behavior.

AcceptToMemoryPool() is called for an invalid coinbase transaction, so setting bypass_limits to true or false has no impact on the test. The only way that changing bypass_limits from true to false could change the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY). Since the ATMP call in this test results in INVALID(TX_CONSENSUS) both before and after this change, there is no change in behavior.

"This logic is not necessary for memory pool transactions, as AcceptToMemoryPool already refuses previously-known transaction ids entirely." refers to the logic at https://github.com/bitcoin/bitcoin/blob/a206b0ea12eb4606b93323268fc81a4f1f952531/src/main.cpp#L484-L486, which was later removed in commit 450cbb0.

…rror string User-facing error messages should not leak internal implementation details like function names. Update the MEMPOOL_REJECTED error string from "Transaction rejected by AcceptToMemoryPool" to the more generic "Transaction rejected by mempool". Also update the MEMPOOL_ERROR error message from "AcceptToMemoryPool failed" to the more precise "Mempool internal error" since this error indicates and internal (e.g. logic/hardware/etc) failure, and not a transaction rejection.

This just calls through to AcceptToMemoryPool() internally, and is currently unused. Also add a new transaction validation failure reason TX_NO_MEMPOOL to indicate that there is no mempool.

…action CTxMemPool::check() will carry out internal consistency checks 1/n times, where n is set by the `-checkmempool` configuration option. By default, mempool consistency checks are disabled entirely on mainnet. Therefore, this change has no effect on mainnet nodes running with default configuration. It simply removes the responsibility to trigger mempool consistency checks from net_processing.

jnewbery · 2021-11-03T14:57:50Z

Thanks for the thorough review @ryanofsky! I believe I've addressed all of your review comments.

stickies-v

2c64270 seems to indicate that all AcceptToMemoryPool() calls from outside validation.cpp/h are removed, but it appears there are still two references left. Is this intentional?

jnewbery · 2021-11-03T16:51:53Z

2c64270 seems to indicate that all AcceptToMemoryPool() calls from outside validation.cpp/h are removed, but it appears there are still two references left. Is this intentional?

Yes, see the AcceptToMemoryPool doxygen comment:

 * Try to add a transaction to the mempool. This is an internal function and is
 * exposed only for testing. Client code should use ChainstateManager::ProcessTransaction()

Those two remaining calls to ATMP are the tests that are referenced in that comment. They can't use the ChainstateManager::ProcessTransaction() interface function since they're testing a mempool that isn't owned by a CChainState.

lsilva01

tACK 0fdb619 on Ubuntu 20.04

This PR successfully removes calls to AcceptToMemoryPool() from the net_processing layer and allocates them to the validation layer (in the new ProcessTransaction method and in ActivateBestChainStep).

It also adds a mempool.check() call after processing a new transaction. It has no effect on the default setting as m_check_ratio will be 0 and CTxMemPool ::check() will return immediately.

txvalidation_tests and txvalidationcache_tests ran successfully after changing bypass_limits from true to false.

theStack

Code-review ACK 0fdb619

Nice improvement! Happy to see a more consistent higher-level interface for adding transactions to the mempool via ChainstateManager, with an automatic CTxMemPool::check() included (which has no effect on mainnet by default, i.e. possible performance losses don't affect users).

ryanofsky

Code review ACK 0fdb619. Only changes since last review: splitting & joining commits, adding more explanations to commit messages, tweaking MEMPOOL_ERROR string, fixing up argument name comments.

This might be ready for merge. It's also a pretty straightforward review if anyone else wants to take a look.

123f5de Remove calls to global Params() in tx_pool test (lsilva01) 9360778 Remove AcceptToMemoryPoolWithTime (lsilva01) Pull request description: This PR refactors AcceptToMemoryPool. . Remove `AcceptToMemoryPoolWithTime` (after #23173, this function is no longer needed). . Remove the `CChainParams chainparams` parameter from ATMP as they can be inferred from the current chain state. . Update the `tx_pool` test with new function signature. ACKs for top commit: jnewbery: reACK 123f5de glozow: light code review ACK 123f5de theStack: re-ACK 123f5de Tree-SHA512: b672d9f091f5fcb4d163c0a443aa469b2ce6f37201136803201c620c3fa329ffb41abd50c5b528787fd1c47e8e09af5ec20ddaa0a4235352c9c619e5691dddb6

f1f10c0 Remove CTxMemPool params from ATMP (lsilva01) Pull request description: Remove `CTxMemPool` parameter from `AcceptToMemoryPool` function, as suggested in bitcoin/bitcoin#23437 (comment) . This requires that `CChainState` has access to `MockedTxPool` in `tx_pool.cpp` as mentioned bitcoin/bitcoin#23173 (comment). So the `MockedTxPool` is attributed to `CChainState::m_mempool` before calling `AcceptToMemoryPool`. Requires #23437. ACKs for top commit: jnewbery: utACK f1f10c0 MarcoFalke: review ACK f1f10c0 🔙 Tree-SHA512: 2a4885f4645014fc1fa98bb1090f13721c1a0796bc0021b9cb43bc8cc13920b6eaf057d1f5ed796e0a110e7813e41fe0196334ce7c80d1231fc057a9a3bdf349

f1f10c0 Remove CTxMemPool params from ATMP (lsilva01) Pull request description: Remove `CTxMemPool` parameter from `AcceptToMemoryPool` function, as suggested in bitcoin#23437 (comment) . This requires that `CChainState` has access to `MockedTxPool` in `tx_pool.cpp` as mentioned bitcoin#23173 (comment). So the `MockedTxPool` is attributed to `CChainState::m_mempool` before calling `AcceptToMemoryPool`. Requires bitcoin#23437. ACKs for top commit: jnewbery: utACK f1f10c0 MarcoFalke: review ACK f1f10c0 🔙 Tree-SHA512: 2a4885f4645014fc1fa98bb1090f13721c1a0796bc0021b9cb43bc8cc13920b6eaf057d1f5ed796e0a110e7813e41fe0196334ce7c80d1231fc057a9a3bdf349

efbb49e Remove CTxMemPool params from ATMP (lsilva01) Pull request description: Remove `CTxMemPool` parameter from `AcceptToMemoryPool` function, as suggested in bitcoin/bitcoin#23437 (comment) . This requires that `CChainState` has access to `MockedTxPool` in `tx_pool.cpp` as mentioned bitcoin/bitcoin#23173 (comment). So the `MockedTxPool` is attributed to `CChainState::m_mempool` before calling `AcceptToMemoryPool`. Requires #23437. ACKs for top commit: jnewbery: utACK efbb49e MarcoFalke: review ACK efbb49e 🔙 Tree-SHA512: 2a4885f4645014fc1fa98bb1090f13721c1a0796bc0021b9cb43bc8cc13920b6eaf057d1f5ed796e0a110e7813e41fe0196334ce7c80d1231fc057a9a3bdf349

Summary: > [test] Don't set bypass_limits to true in txvalidatiocache_tests.cpp > > AcceptToMemoryPool() is called for transactions with fees above > minRelayTxFee and with the mempool not full, so setting bypass_limits to > true or false has no impact on the test. > > The only way that changing bypass_limits from true to false could change > the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY). > Since all the ATMP calls in this test result in VALID both before and > after this change, there is no change in behavior. bitcoin/bitcoin@497c9e2 > [test] Don't set bypass_limits to true in txvalidation_tests.cpp > > AcceptToMemoryPool() is called for an invalid coinbase transaction, so > setting bypass_limits to true or false has no impact on the test. > > The only way that changing bypass_limits from true to false could change > the result would be to change the outcome to INVALID(TX_MEMPOOL_POLICY). > Since the ATMP call in this test results in INVALID(TX_CONSENSUS) both > before and after this change, there is no change in behavior. This is a backport of [[bitcoin/bitcoin#23173 | core#23173]] bitcoin/bitcoin@5759fd1 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12232

Summary: > "This logic is not necessary for memory pool transactions, as > AcceptToMemoryPool already refuses previously-known transaction ids > entirely." refers to the logic at > https://github.com/bitcoin/bitcoin/blob/a206b0ea12eb4606b93323268fc81a4f1f952531/src/main.cpp#L484-L486, > which was later removed in commit rABC450cbb0944cd20a06ce806e6679a1f4c83c50db2. This is a backport of [[bitcoin/bitcoin#23173 | core#23173]] bitcoin/bitcoin@4c24142 Test Plan: NA Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12233

…rror string Summary: User-facing error messages should not leak internal implementation details like function names. Update the MEMPOOL_REJECTED error string from "Transaction rejected by AcceptToMemoryPool" to the more generic "Transaction rejected by mempool". Also update the MEMPOOL_ERROR error message from "AcceptToMemoryPool failed" to the more precise "Mempool internal error" since this error indicates and internal (e.g. logic/hardware/etc) failure, and not a transaction rejection. These error messages are not covered by tests because they are overwritten with more precise error messages from the ATMP validation state in `sendrawtransaction`. This is a backport of [[bitcoin/bitcoin#23173 | core#23173]] bitcoin/bitcoin@36167fa Test Plan: `ninja all check check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12234

…outside validation Summary: Also add a new transaction validation failure reason TX_NO_MEMPOOL to indicate that there is no mempool. This is a backport of [[bitcoin/bitcoin#23173 | core#23173]] bitcoin/bitcoin@92a3aee bitcoin/bitcoin@2c64270 Note: due to D1667, there are calls to AcceptToMemoryPool in `DisconnectedBlockTransactions::updateMempoolForReorg` that cannot access the chain manager. However `updateMempoolForReorg` is only called from within validation.cpp, so it should not be a problem. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12235

…action Summary: CTxMemPool::check() will carry out internal consistency checks 1/n times, where n is set by the `-checkmempool` configuration option. By default, mempool consistency checks are disabled entirely on mainnet. Therefore, this change has no effect on mainnet nodes running with default configuration. It simply removes the responsibility to trigger mempool consistency checks from net_processing. This concludes a backport of [[bitcoin/bitcoin#23173 | core#23173]] bitcoin/bitcoin@0fdb619 Depends on D12235 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12236

jnewbery mentioned this pull request Oct 4, 2021

txmempool -/-> validation 1/2: improve performance of check() and remove dependency on validation #23157

Merged

maflcko reviewed Oct 4, 2021

View reviewed changes

src/test/util/setup_common.cpp Outdated Show resolved Hide resolved

DrahtBot added P2P RPC/REST/ZMQ Utils/log/libs Validation labels Oct 4, 2021

jnewbery force-pushed the 2021-09-process-transaction branch from 87a286d to 58538ff Compare October 4, 2021 16:08

maflcko added Refactoring and removed RPC/REST/ZMQ P2P Utils/log/libs Validation labels Oct 4, 2021

glozow reviewed Oct 5, 2021

View reviewed changes

jnewbery force-pushed the 2021-09-process-transaction branch from 58538ff to 6b2c8c8 Compare October 7, 2021 09:50

jnewbery force-pushed the 2021-09-process-transaction branch from 6b2c8c8 to 1d20d43 Compare October 7, 2021 13:58

glozow reviewed Oct 12, 2021

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

promag reviewed Oct 17, 2021

View reviewed changes

jnewbery force-pushed the 2021-09-process-transaction branch from 1d20d43 to 3bb34f0 Compare October 19, 2021 17:09

glozow reviewed Oct 20, 2021

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.h Outdated Show resolved Hide resolved

jnewbery added 7 commits November 3, 2021 12:04

[validation] Add CChainState::ProcessTransaction()

92a3aee

This just calls through to AcceptToMemoryPool() internally, and is currently unused. Also add a new transaction validation failure reason TX_NO_MEMPOOL to indicate that there is no mempool.

[refactor] Don't call AcceptToMemoryPool() from outside validation.cpp

2c64270

jnewbery force-pushed the 2021-09-process-transaction branch from f87e07c to 0fdb619 Compare November 3, 2021 14:57

stickies-v reviewed Nov 3, 2021

View reviewed changes

lsilva01 approved these changes Nov 3, 2021

View reviewed changes

lsilva01 mentioned this pull request Nov 4, 2021

refactor: AcceptToMemoryPool #23437

Merged

theStack approved these changes Nov 5, 2021

View reviewed changes

DrahtBot mentioned this pull request Nov 6, 2021

Erlay: bandwidth-efficient transaction relay protocol #21515

Closed

ryanofsky approved these changes Nov 9, 2021

View reviewed changes

maflcko merged commit 38b2a0a into bitcoin:master Nov 10, 2021

jnewbery deleted the 2021-09-process-transaction branch November 10, 2021 13:55

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 10, 2021

Merge bitcoin#23173: Add ChainstateManager::ProcessTransaction

12efce2

lsilva01 mentioned this pull request Nov 13, 2021

Remove CTxMemPool params from ATMP #23465

Merged

bitcoin locked and limited conversation to collaborators Nov 10, 2022

Add ChainstateManager::ProcessTransaction #23173

Add ChainstateManager::ProcessTransaction #23173

Uh oh!

Conversation

jnewbery commented Oct 4, 2021

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Oct 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jamesob commented Oct 4, 2021

Uh oh!

jnewbery commented Oct 4, 2021

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Oct 5, 2021

Uh oh!

jnewbery commented Oct 7, 2021

Uh oh!

jnewbery commented Oct 7, 2021

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Oct 19, 2021

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnewbery commented Nov 3, 2021

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Nov 3, 2021

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Add `ChainstateManager::ProcessTransaction` #23173

Add `ChainstateManager::ProcessTransaction` #23173

DrahtBot commented Oct 4, 2021 •

edited

Loading