validation/coins: limit MemPoolAccept access to coins cache + make it responsible for uncaching

glozow commented at 11:33 pm on February 10, 2021: member

This PR is a subpart of #20833 which enables package validation through testmempoolaccept. The first commit addresses some leftover comments from #21062.

When validating unconfirmed transactions, we want to account for all coins brought in from disk so we can uncache them if the tx is invalid. The disconnection of CCoinsViews right now doesn’t quite achieve what’s it’s trying to do (prevent MemPoolAccept from unnecessarily accessing the coins cache and forgetting to uncache later). This PR improves it by doing 2 things: (1) Disconnect m_viewmempool from the coins cache when we don’t need it to be connected. (2) Make MemPoolAccept destructor clean up after itself by uncaching coins accessed, instead of leaving it up to the caller. Only leave the coins cached when a transaction is accepted to mempool. In the future, if someone edits the MemPoolAccept class and forgets to consider uncaching, the default behavior is to uncache (which is safer).

fanquake added the label Validation on Feb 10, 2021

DrahtBot commented at 5:56 am on February 11, 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:

#20833 (rpc/validation: enable packages through testmempoolaccept 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.

glozow force-pushed on Feb 11, 2021

in src/validation.cpp:708 in 14c94b30f1 outdated

690@@ -671,7 +691,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
691     // we have all inputs cached now, so switch back to dummy (to protect
692     // against bugs where we pull more inputs from disk that miss being added
693     // to coins_to_uncache)
694-    m_view.SetBackend(m_dummy);
695+    m_viewmempool.SetBackend(m_dummy);

glozow commented at 10:20 pm on February 11, 2021:

☝️ this is the main change 👆

jnewbery commented at 2:25 pm on February 22, 2021:

You switch m_view.SetBackend(m_dummy) to m_viewmempool.SetBackend(m_dummy) in commit [validation] disconnect m_viewmempool from coins cache when unnecessary.

I think perhaps in that commit you should just add m_viewmempool.SetBackend(m_dummy), and then in the subsequent commit ([validation] keep m_view->m_viewmempool connected), you should remove m_view.SetBackend(m_dummy). That keeps the commits more separated (the first is adding the m_viewmempool // CoinsTip() split, and the second is removing the m_view // m_viewmempool split).

glozow commented at 5:33 pm on February 22, 2021:

Good point! Done in the last push

glozow renamed this:
~~[WIP] validation/coins: limit MemPoolAccept access to coins cache~~
validation/coins: limit MemPoolAccept access to coins cache + make it responsible for uncaching
on Feb 11, 2021

glozow marked this as ready for review on Feb 11, 2021

ariard commented at 2:02 pm on February 12, 2021: member

Do you think you can take the following diff in the doc followup commit ?

 0diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
 1index ac4240447..f8f4a6d94 100644
 2--- a/src/rpc/rawtransaction.cpp
 3+++ b/src/rpc/rawtransaction.cpp
 4@@ -896,7 +896,7 @@ static RPCHelpMan testmempoolaccept()
 5                         {
 6                             {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
 7                             {RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"},
 8-                            {RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"},
 9+                            {RPCResult::Type::BOOL, "allowed", "If the mempool and client specified maxfeerate allows this tx to be inserted"},
10                             {RPCResult::Type::NUM, "vsize", "Virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted (only present when 'allowed' is true)"},
11                             {RPCResult::Type::OBJ, "fees", "Transaction fees (only present if 'allowed' is true)",
12                             {

That would close my #21704 concerns for now :) ?

laanwj added this to the "Blockers" column in a project

DrahtBot added the label Needs rebase on Feb 20, 2021

doc/style followups in MempoolAcceptResult 56cfa1c127

glozow force-pushed on Feb 20, 2021

glozow commented at 5:57 pm on February 20, 2021: member

Rebased

DrahtBot removed the label Needs rebase on Feb 20, 2021

jnewbery commented at 2:39 pm on February 22, 2021: member

Final code looks good. I have one comment inline about the commit structure.

There’s a small change in the final commit that you haven’t communicated: currently, if a testmempoolaccept is successful, we keep any fetched coins in the cache. After this commit we don’t. I think that’s a fine change - if the transaction hasn’t been added to the mempool, then there’s no reason for the coins it spends to be added to the cache. The change is perhaps worth pointing out in the commit log though.

glozow force-pushed on Feb 22, 2021

glozow commented at 5:49 pm on February 22, 2021: member

Thanks for the review @jnewbery! Addressed your comments. Added some sentences about the uncaching behavior to https://github.com/bitcoin/bitcoin/commit/acef823e895b80fd7f4c2f5a44c3470fe46c1a6a commit log.

achow101 commented at 7:34 pm on February 22, 2021: member

ACK acef823e895b80fd7f4c2f5a44c3470fe46c1a6a

in src/validation.cpp:1104 in acef823e89 outdated

1116-        // invalid transactions that attempt to overrun the in-memory coins cache
1117-        // (`CCoinsViewCache::cacheCoins`).
1118 
1119-        for (const COutPoint& hashTx : coins_to_uncache)
1120-            active_chainstate.CoinsTip().Uncache(hashTx);
1121-    }

ariard commented at 5:54 pm on February 23, 2021:

I would rather keep coin cache cleanup at this location. After this change, it’s spread in two locations : in case of success evaluation in AcceptSingleTransaction, in case of failure in MemPoolAccept destructor. A reviewer has to read both locations now.

Further, it’s harder to qualify that cleanup is reserved for success path in AcceptSingleTransaction, considering that MemPoolAcceptResult internal state is provided by Workspace.

Also, I don’t know about relying on RAII semantic, the destructor will be only called when object is going out of scope. In that case, does it mean we’ll only cleanup when caller ends up ? For e.g, if the caller was BroadcastTransaction, may the coin cache cleanup on cs_main lock tacking before RelayTransaction ?

glozow commented at 7:31 pm on February 23, 2021:

After this change, it’s spread in two locations : in case of success evaluation in AcceptSingleTransaction, in case of failure in MemPoolAccept destructor.

Not really. The only location a reviewer has to check is wherever they want to keep the coins in cache. Also, the change here means that if someone forgets to consider uncaching, it defaults to uncache all. This is safer.

Also, I don’t know about relying on RAII semantic, the destructor will be only called when object is going out of scope. In that case, does it mean we’ll only cleanup when caller ends up ? For e.g, if the caller was BroadcastTransaction, may the coin cache cleanup on cs_main lock tacking before RelayTransaction ?

The two possible callers of MemPoolAccept are ATMP and ProcessNewPackage They invoke it like this:

0const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args);

afaik it goes out of scope immediately, before the lock is released.

ariard commented at 1:14 pm on February 24, 2021:

Not really. The only location a reviewer has to check is wherever they want to keep the coins in cache.

Depends what you’re willingly to convince yourself by reviewing. Assuming uncaching in case of failures is the correct behavior, looking at the proposed changes, you have to a) verify that uncaching code is well-implemented in MemPoolAccept destructor, b) identify that caching conservation is covering all success paths and thus dissociate success code path from errors one in AcceptSingleTransaction and c) ensure the RAAI semantic is enforced as intended. IMHO, more complex than current one-place location.

That said, I think the code is correct, and each one review process and code style flavors might turn as really abstract discussion so not going to argue further :)

jnewbery commented at 4:41 pm on February 24, 2021:

The two possible callers of MemPoolAccept are ATMP and ProcessNewPackage They invoke it like this:

const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args);

afaik it goes out of scope immediately, before the lock is released.

I believe you’re right. In that expression, MemPoolAccept() is a temporary, and the lifetime rules for temporaries are

All temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created, and if multiple temporary objects were created, they are destroyed in the order opposite to the order of creation. This is true even if that evaluation ends in throwing an exception.

https://en.cppreference.com/w/cpp/language/lifetime#Temporary_object_lifetime

Note that there are two exceptions to that rule, neither of which apply here:

The lifetime of a temporary object may be extended by binding to a const lvalue reference or to an rvalue reference (since C++11), see reference initialization for details.

The lifetime of a temporary object created when evaluating the default arguments of a default constructor used to initialize an element of an array ends before the next element of the array begins initialization.

ariard commented at 6:23 pm on February 23, 2021: member

Should you extend 34ab1ad7 commit message to explain why it’s safe to access output in the mempool ? I would say for the mempool, we don’t have overrun concerns, all the state is already in-memory. No size extension from disk is possible.

in src/validation.cpp:1107 in acef823e89 outdated

1103@@ -1088,20 +1104,10 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp
1104                                                       bool bypass_limits, bool test_accept)
1105                                                       EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1106 {
1107-    std::vector<COutPoint> coins_to_uncache;
1108-    MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, coins_to_uncache, test_accept };
1109-
1110+    MemPoolAccept::ATMPArgs args { chainparams, nAcceptTime, bypass_limits, test_accept };

jnewbery commented at 4:43 pm on February 24, 2021:

You can const this if you want

glozow commented at 5:40 pm on February 25, 2021:

I always wanna const

in src/validation.cpp:1111 in acef823e89 outdated

1117-        // (`CCoinsViewCache::cacheCoins`).
1118 
1119-        for (const COutPoint& hashTx : coins_to_uncache)
1120-            active_chainstate.CoinsTip().Uncache(hashTx);
1121-    }
1122     // After we've (potentially) uncached entries, ensure our coins cache is still within its size limits

jnewbery commented at 4:43 pm on February 24, 2021:

Consider removing “After we’ve (potentially) uncached entries”. That was referring to the line above. Now that it’s removed, that part of the comment is not particularly relevant here.

jnewbery commented at 4:47 pm on February 24, 2021: member

utACK acef823e895b80fd7f4c2f5a44c3470fe46c1a6a

[validation] disconnect m_viewmempool from coins cache when unnecessary

The hierarchy of CoinsViews here is:
m_view -> m_viewmempool -> chainstate coins cache

To prevent DoS, we want to limit access to coins cache and ensure we
uncache coins if the transaction turns out to be invalid.  When
m_viewmempool is always connected to the coins cache, it's harder reason
about this. We could accidentally access the coins cache if we use
m_viewmempool for something.

8f52c6e3c9

[validation] make MemPoolAccept responsible for uncaching coins

MemPoolAccept should clean up after itself (woohoo RAII), uncache all
coins by default, and explicitly decide to keep them on specific
occasions (i.e. when tx is submitted to mempool). This is safer and
easier to reason about than leaving it up to the caller.
Also allows us to make ATMPArgs always const.

Note: with this commit, we always uncache coins after a test_accept
regardless of validation results. Previously, we uncached whenever a tx
failed validation.

aeaf5407c3

glozow force-pushed on Feb 25, 2021

glozow commented at 5:51 pm on February 25, 2021: member

I’m working on #20833 and realized I don’t need to keep m_view and m_viewmempool connected for it to work 😛 so I took out the 3rd commit ([validation] keep m_view->m_viewmempool connected). @ariard you might find this PR a bit more acceptable now :). I’ll try to add #21146 (comment) to a different PR that’s more relevant to the RPC code.

Also addressed @jnewbery’s comments #21146 (review) and #21146 (review).

glozow commented at 6:27 pm on March 2, 2021: member

I still think this would be a nice change but since I don’t need this for #20833 anymore, I’m closing this to focus on that instead.

glozow closed this on Mar 2, 2021

jnewbery removed this from the "Blockers" column in a project

DrahtBot locked this on Aug 16, 2022

validation/coins: limit MemPoolAccept access to coins cache + make it responsible for uncaching #21146

Conflicts