Add `ChainstateManager::ProcessTransaction`

jnewbery commented at 1:23 pm on October 4, 2021: member

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.

in src/test/util/setup_common.cpp:318 in 87a286d0cb outdated

314@@ -315,7 +315,7 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio
315     // If submit=true, add transaction to the mempool.
316     if (submit) {
317         LOCK(cs_main);
318-        const MempoolAcceptResult result = AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), *m_node.mempool.get(), MakeTransactionRef(mempool_txn), /* bypass_limits */ false);
319+        const MempoolAcceptResult result = m_node.chainman->ProcessTransaction(MakeTransactionRef(mempool_txn), /* bypass_limits */ false);

MarcoFalke commented at 1:31 pm on October 4, 2021:

wrong

0        const MempoolAcceptResult result = m_node.chainman->ProcessTransaction(MakeTransactionRef(mempool_txn), /* test_accept */ false);

jnewbery commented at 4:08 pm on October 4, 2021:

Fixed!

MarcoFalke commented at 1:31 pm on October 4, 2021: member

Concept ACK

DrahtBot added the label P2P on Oct 4, 2021

DrahtBot added the label RPC/REST/ZMQ on Oct 4, 2021

DrahtBot added the label Utils/log/libs on Oct 4, 2021

DrahtBot added the label Validation on Oct 4, 2021

DrahtBot commented at 2:22 pm on October 4, 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:

#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 commented at 2:47 pm on October 4, 2021: member

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

jnewbery force-pushed on Oct 4, 2021

jnewbery commented at 4:09 pm on October 4, 2021: member

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

MarcoFalke removed the label RPC/REST/ZMQ on Oct 4, 2021

MarcoFalke removed the label P2P on Oct 4, 2021

MarcoFalke removed the label Utils/log/libs on Oct 4, 2021

MarcoFalke added the label Refactoring on Oct 4, 2021

MarcoFalke removed the label Validation on Oct 4, 2021

glozow commented at 10:17 am on October 5, 2021: member

Concept ACK

practicalswift commented at 10:21 am on October 5, 2021: contributor

Concept ACK

jnewbery force-pushed on Oct 7, 2021

jnewbery commented at 9:51 am on October 7, 2021: member

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 force-pushed on Oct 7, 2021

jnewbery commented at 2:00 pm on October 7, 2021: member

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).

in src/validation.cpp:333 in 179e0fb145 outdated

329@@ -330,54 +330,6 @@ static bool IsCurrentForFeeEstimation(CChainState& active_chainstate) EXCLUSIVE_
330     return true;
331 }
332 
333-void CChainState::MaybeUpdateMempoolForReorg(

glozow commented at 3:24 pm on October 11, 2021:

could you explain why this move is needed? is it to reduce the diff? 179e0fb1456cf7f75517d537ba68774d59312311 Move CChainState::MaybeUpdateMempoolForReorg() lower in validation.cpp

jnewbery commented at 1:53 pm on October 19, 2021:

Oops. This is leftover from a previous version of the branch where I made ATMP static in validation.cpp and removed the declaration from validation.h. That meant moving all of the callers of ATMP to be lower in the file (or adding a forward declaration).

I’ve now left ATMP in the header as a test-only function, so this commit is no longer required.

in src/validation.h:208 in 1d20d4330e outdated

204@@ -205,11 +205,7 @@ struct PackageMempoolAcceptResult
205         : m_tx_results{ {wtxid, result} } {}
206 };
207 
208-/**
209- * (Try to) add a transaction to the memory pool.
210- * @param[in]  bypass_limits   When true, don't enforce mempool fee limits.
211- * @param[in]  test_accept     When true, run validation checks but don't submit to mempool.
212- */
213+/** Internal function. Exposed only for fuzz testing. */

glozow commented at 3:29 pm on October 11, 2021:

Why can’t ATMP be static and ProcessTransaction be called from the fuzz test instead?

glozow commented at 8:30 am on October 12, 2021:

I also wonder if, now that AcceptToMemoryPool is nothing but a wrapper for AcceptToMemoryPoolWithTime, we could just delete ATMP and call ATMPWT. It would also make it easier to make sure we’reremoving mentions of ATMP throughout the codebase.

jnewbery commented at 1:56 pm on October 19, 2021:

The fuzz test is using a derived class of the mempool called MockedTxPool, which isn’t referenced from the CChainState object, and so it can’t use the ChainstateManager interface to submit transactions to the mempool.

It may be possible to clean this up in future so the fuzz test is only using public interfaces, but I think that can be left for another PR.

I also wonder if, now that AcceptToMemoryPool is nothing but a wrapper for AcceptToMemoryPoolWithTime, we could just delete ATMP and call ATMPWT.

I agree that we can flatten those function calls. I’d prefer to remove ATMP and then rename ATMPWT to ATMP and update the call sites, but I think we can leave that for a follow-up PR.

in src/validation.h:1003 in 93bdbf468a outdated

 996@@ -997,6 +997,15 @@ class ChainstateManager
 997      */
 998     bool ProcessNewBlockHeaders(const std::vector<CBlockHeader>& block, BlockValidationState& state, const CChainParams& chainparams, const CBlockIndex** ppindex = nullptr) LOCKS_EXCLUDED(cs_main);
 999 
1000+    /**
1001+     * (Try to) add a transaction to the memory pool.
1002+     *
1003+     * @param[in]  bypass_limits   When true, don't enforce mempool fee limits.

glozow commented at 3:30 pm on October 11, 2021:

93bdbf468addd11581a5f1a4465147da24920235 adds a doxygen comment that is deleted 3 commits later. Might not be necessary…

jnewbery commented at 5:09 pm on October 19, 2021:

Not necessary, but might be confusing for reviewers if I add a doxygen comment in 93bdbf4 that doesn’t document all the parameters.

glozow commented at 8:30 am on October 12, 2021: member

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.

promag commented at 10:43 am on October 17, 2021: member

Concept ACK.

Light code review ACK 1d20d4330e21ab600747ced872eba372a08086c5.

jnewbery force-pushed on Oct 19, 2021

jnewbery commented at 5:14 pm on October 19, 2021: member

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

in src/validation.h:215 in 3bb34f0c71 outdated

213+ * exposed only for testing. Client code should use ChainstateManager::ProcessTransaction()
214+ *
215+ * @param[in]  active_chainstate  Reference to the active chainstate.
216+ * @param[in]  pool               Reference to the node's mempool.
217+ * @param[in]  tx                 The transaction to submit for mempool acceptance.
218+ * @param[in]  bypass_limits      When true, don't enforce mempool fee limits.

glozow commented at 4:17 pm on October 20, 2021:

bypass_limits also allows the mempool to grow past the maximum size: https://github.com/bitcoin/bitcoin/blob/3bb34f0c7104dd5bfc141b6c24ab206c08f1b8c7/src/validation.cpp#L914

jnewbery commented at 10:52 am on October 29, 2021:

Thanks. Fixed!

in src/validation.cpp:3433 in 3bb34f0c71 outdated

3438+    if (!active_chainstate.m_mempool) {
3439+        TxValidationState state;
3440+        state.Invalid(TxValidationResult::TX_NO_MEMPOOL);
3441+        return MempoolAcceptResult::Failure(state);
3442+    }
3443+    auto result = AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, /* bypass_limits= */ false, test_accept);

glozow commented at 4:17 pm on October 20, 2021:

nit in f21c86c6fa

0    const auto result = AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, /* bypass_limits= */ false, test_accept);

glozow commented at 4:24 pm on October 20, 2021:

The addition of the /* bypass_limits= */ comment should be moved from 90f1a1f8cf to f21c86c6fa

jnewbery commented at 10:52 am on October 29, 2021:

I don’t agree. Prior to 90f1a1f8cf8ba4a11b2f93b55d1c0e25bcdcdd6f, the argument passed here is a named variable bypass_limits. In that commit we change it to be the literal false, so adding the /* bypass_limits= */ comment makes it obvious which argument we’re passing.

in src/validation.cpp:353 in 3bb34f0c71 outdated

349@@ -350,7 +350,7 @@ void CChainState::MaybeUpdateMempoolForReorg(
350         // ignore validation errors in resurrected transactions
351         if (!fAddToMempool || (*it)->IsCoinBase() ||
352             AcceptToMemoryPool(
353-                *this, *m_mempool, *it, true /* bypass_limits */).m_result_type !=
354+                *this, *m_mempool, *it, true /* bypass_limits */, /* test_accept= */ false).m_result_type !=

glozow commented at 4:19 pm on October 20, 2021:

in aad911d28b: I don’t disagree with this change (in fact it would be nice to remove the default argument in the future since it’s common to mix up the two bools), but it’s not related to this commit.

jnewbery commented at 10:52 am on October 29, 2021:

You’re right. I’ve removed it and will leave for a future cleanup.

in src/validation.h:1014 in 3bb34f0c71 outdated

1009+     * Try to add a transaction to the memory pool.
1010+     *
1011+     * @param[in]  tx              The transaction to submit for mempool acceptance.
1012+     * @param[in]  test_accept     When true, run validation checks but don't submit to mempool.
1013+     */
1014+    MempoolAcceptResult ProcessTransaction(const CTransactionRef& tx, bool test_accept=false)

glozow commented at 4:34 pm on October 20, 2021:

in 48e69da541: it would be nice for the compiler to let us know we aren’t doing all this validation work for nothing, how about a lil dash of C++17 spice?

0    [[nodiscard]] MempoolAcceptResult ProcessTransaction(const CTransactionRef& tx, bool test_accept=false)

jnewbery commented at 10:52 am on October 29, 2021:

Done! :hot_pepper:

glozow commented at 4:38 pm on October 20, 2021: member

code review ACK 3bb34f0c71

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

DrahtBot added the label Needs rebase on Oct 25, 2021

jnewbery force-pushed on Oct 29, 2021

jnewbery commented at 10:53 am on October 29, 2021: member

Thanks for the review, @glozow! I’ve rebased past #23157 and addressed all your comments.

DrahtBot removed the label Needs rebase on Oct 29, 2021

jnewbery added the label Validation on Oct 29, 2021

in src/test/txvalidation_tests.cpp:41 in 23e5cba774 outdated

37@@ -38,7 +38,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup)
38 
39     unsigned int initialPoolSize = m_node.mempool->size();
40     const MempoolAcceptResult result = AcceptToMemoryPool(m_node.chainman->ActiveChainstate(), *m_node.mempool, MakeTransactionRef(coinbaseTx),
41-                true /* bypass_limits */);
42+                false /* bypass_limits */);

ryanofsky commented at 9:26 pm on November 1, 2021:

In commit “[test] Don’t set bypass_limits to true in txvalidation_tests.cpp” (23e5cba7749ae0a668c0e3b084e65f723617ba9f)

Note, just for understanding: I think this change is safe and doesn’t risk making the test meaningless because checks below verify it fails TX_CONSENSUS, not TX_MEMPOOL_POLICY. Might be worth saying this in the commit description, though, if you happen to update it.

jnewbery commented at 2:57 pm on November 3, 2021:

That’s correct. I’ve updated the commit log as suggested.

in src/util/error.cpp:25 in edd4b472ad outdated

19@@ -20,9 +20,9 @@ bilingual_str TransactionErrorString(const TransactionError err)
20         case TransactionError::P2P_DISABLED:
21             return Untranslated("Peer-to-peer functionality missing or disabled");
22         case TransactionError::MEMPOOL_REJECTED:
23-            return Untranslated("Transaction rejected by AcceptToMemoryPool");
24+            return Untranslated("Transaction rejected by mempool");
25         case TransactionError::MEMPOOL_ERROR:
26-            return Untranslated("AcceptToMemoryPool failed");
27+            return Untranslated("Mempool logic error");

ryanofsky commented at 9:45 pm on November 1, 2021:

In commit “[refactor] Don’t call AcceptToMemoryPool() from outside validation.cpp” (edd4b472adbe6f45aee40d739f6c1f44e2dc35e2)

Are these changes part of the wrong commit? I wouldn’t expect to see changing error messages in a refactoring commit.

jnewbery commented at 2:57 pm on November 3, 2021:

That’s a fair point. I’ve split this change into its own commit with the reasoning in the commit log.

ryanofsky commented at 9:19 pm on November 9, 2021:

re: #23173 (review)

I’ve split this change into its own commit with the reasoning in the commit log.

Not sure if it’s intentional, but part of edd4b472adbe6f45aee40d739f6c1f44e2dc35e2 that was changing “transaction pool” to “mempool” in some comments was also dropped. Seem fine but just checking it wasn’t lost accidentally.

in src/validation.cpp:3433 in bbc785f31b outdated

3428+                                                          bool test_accept)
3429+{
3430+    CChainState& active_chainstate = ActiveChainstate();
3431+    if (!active_chainstate.m_mempool) {
3432+        TxValidationState state;
3433+        state.Invalid(TxValidationResult::TX_NO_MEMPOOL);

ryanofsky commented at 9:49 pm on November 1, 2021:

In commit “[validation] Add CChainState::ProcessTransaction()” (bbc785f31b5d70c702ae37d259c46652e243ddba)

Is it intentional to not pass reject reason & debug message strings to Invalid()? Depending on when this is expected to happen, strings might be more friendly than a numeric error code.

jnewbery commented at 2:57 pm on November 3, 2021:

Good point. Fixed!

in src/validation.cpp:1662 in bc9218139f outdated

1657@@ -1658,8 +1658,6 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state,
1658     // can be duplicated to remove the ability to spend the first instance -- even after
1659     // being sent to another address.
1660     // See BIP30, CVE-2012-1909, and http://r6.ca/blog/20120206T005236Z.html for more information.
1661-    // This logic is not necessary for memory pool transactions, as AcceptToMemoryPool
1662-    // already refuses previously-known transaction ids entirely.

ryanofsky commented at 9:54 pm on November 1, 2021:

In commit “[validation] Remove comment about AcceptToMemoryPool()” (bc9218139fd17d26eaac5093ac36b7da016734b6)

Nice find!

jnewbery commented at 2:57 pm on November 3, 2021:

:bow:

in src/validation.cpp:3433 in f87e07c6fe outdated

3429@@ -3430,7 +3430,9 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef&
3430         state.Invalid(TxValidationResult::TX_NO_MEMPOOL);
3431         return MempoolAcceptResult::Failure(state);
3432     }
3433-    return AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, /* bypass_limits= */ false, test_accept);
3434+    auto result = AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, /* bypass_limits= */ false, test_accept);
3435+    active_chainstate.m_mempool->check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1);

ryanofsky commented at 10:06 pm on November 1, 2021:

In commit “[validation] Always call mempool.check() after processing a new transaction” (f87e07c6fe321f0fb97703c82c0e4122f800589f)

This change does not appear to be a pure refactoring, so it would be helpful if commit description mentioned something about cases when these new checks will be done and whether there are effects on validation or performance.

jnewbery commented at 2:57 pm on November 3, 2021:

For mainnet nodes with default config, there’s no behaviour change. I’ve added the following text to the commit log:

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.

in src/validation.h:1004 in bbc785f31b outdated

 999+     *
1000+     * @param[in]  tx              The transaction to submit for mempool acceptance.
1001+     * @param[in]  bypass_limits   When true, don't enforce mempool fee and capacity limits.
1002+     * @param[in]  test_accept     When true, run validation checks but don't submit to mempool.
1003+     */
1004+    [[nodiscard]] MempoolAcceptResult ProcessTransaction(const CTransactionRef& tx, bool bypass_limits, bool test_accept=false)

ryanofsky commented at 10:22 pm on November 1, 2021:

In commit “[validation] Add CChainState::ProcessTransaction()” (bbc785f31b5d70c702ae37d259c46652e243ddba)

IMO it would be really good to add this function in its final form without the bypass_limits parameter, so the later commit removing it can be dropped. This would simplify reviewing the PR, and more importantly avoid unsafe change:

0-    [[nodiscard]] MempoolAcceptResult ProcessTransaction(const CTransactionRef& tx, bool bypass_limits, bool test_accept=false)
1+    [[nodiscard]] MempoolAcceptResult ProcessTransaction(const CTransactionRef& tx, bool test_accept=false)

Change is unsafe because a ProcessTransaction(tx, true) call would still compile and silently change meaning from “bypass limits and don’t test” to “don’t bypass limits and do test”

Better to avoid this churn and risk, and just add the new method in final form without temporary (bool, bool) params.

ryanofsky commented at 3:54 pm on November 2, 2021:

In commit “[validation] Add CChainState::ProcessTransaction()” (bbc785f31b5d70c702ae37d259c46652e243ddba)

Maybe for consistency with ProcessNewBlock and ProcessNewBlockHeaders, call this method ProcessNewTransaction instead of ProcessTransaction. Might also help suggest that purpose of processing is to add new data, not validate existing data.

jnewbery commented at 2:56 pm on November 3, 2021:

Haha you noticed. I actually hope that we’d go the other way and change ProcessNewBlock to ProcessBlock and ProcessNewBlockHeaders to ProcessBlockHeaders. There’s nothing to prevent passing in the same block/headers/transaction that have previously been processed, and the functions have ways of returning that information to the caller (ProcessNewBlock will set the new_block out-param to false and ProcessTransaction will set the TxValidationState result to TX_CONFLICT to indicate “Tx already in mempool or conflicts with a tx in the chain”). I think the New in ProcessNew is therefore redundant and perhaps even a bit misleading.

jnewbery commented at 2:56 pm on November 3, 2021:

Good idea. This was a misguided attempt to make the commits as granular as possible, but you’re right that it just adds churn and confusion.

ryanofsky approved

ryanofsky commented at 10:34 pm on November 1, 2021: member

Code review ACK f87e07c6fe321f0fb97703c82c0e4122f800589f with caveat that I’m not sure if there are maybe performance or other implications from adding more mempool checks in the last commit.

All the other changes are straightforward, easy to review, and clean up the code nicely.

As usual, feel free to ignore review suggestions below, which are all minor.

[test] Don't set bypass_limits to true in txvalidationcache_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.

497c9e2964

[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.

5759fd12b8

[validation] Remove comment about AcceptToMemoryPool()

"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 450cbb0944cd20a06ce806e6679a1f4c83c50db2.

4c24142b1e

[logging/documentation] Remove reference to AcceptToMemoryPool from error 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.

36167faea9

[validation] Add CChainState::ProcessTransaction()

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.

92a3aeecf6

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

[validation] Always call mempool.check() after processing a new transaction

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.

0fdb619aaf

jnewbery force-pushed on Nov 3, 2021

jnewbery commented at 2:57 pm on November 3, 2021: member

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

stickies-v commented at 4:42 pm on November 3, 2021: member

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 commented at 4:51 pm on November 3, 2021: member

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:

0 * Try to add a transaction to the mempool. This is an internal function and is
1 * 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 approved

lsilva01 commented at 7:36 pm on November 3, 2021: contributor

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 approved

theStack commented at 3:39 pm on November 5, 2021: member

Code-review ACK 0fdb619aaf1d62598263361a6082d182be1af792

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 approved

ryanofsky commented at 9:26 pm on November 9, 2021: member

Code review ACK 0fdb619aaf1d62598263361a6082d182be1af792. 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.

MarcoFalke merged this on Nov 10, 2021

MarcoFalke closed this on Nov 10, 2021

jnewbery deleted the branch on Nov 10, 2021

sidhujag referenced this in commit 12efce29d6 on Nov 10, 2021

MarcoFalke referenced this in commit 1c06e1a7b6 on Dec 1, 2021

MarcoFalke referenced this in commit 84d921e79c on Dec 8, 2021

sidhujag referenced this in commit ff6e1bdf71 on Dec 8, 2021

RandyMcMillan referenced this in commit 1cd9ee7a8f on Dec 23, 2021

Fabcien referenced this in commit c7b08654f7 on Oct 13, 2022

Fabcien referenced this in commit be412f5142 on Oct 13, 2022

Fabcien referenced this in commit 2096754c7d on Oct 13, 2022

Fabcien referenced this in commit 924ee9eb56 on Oct 13, 2022

Fabcien referenced this in commit 2d2f6e3047 on Oct 13, 2022

DrahtBot locked this on Nov 10, 2022

Add ChainstateManager::ProcessTransaction #23173

Conflicts

Add `ChainstateManager::ProcessTransaction` #23173