rpc/validation: enable packages through testmempoolaccept

glozow commented at 7:16 am on January 3, 2021: member

This PR enables validation dry-runs of packages through the testmempoolaccept RPC. The expectation is that the results returned from testmempoolaccept are what you’d get from test-then-submitting each transaction individually, in that order (this means the package is expected to be sorted in topological order, for now at least). The validation is also atomic: in the case of failure, it immediately halts and may return “unfinished” MempoolAcceptResults for transactions that weren’t fully validated. The API for 1 transaction stays the same.

Motivation:

This allows you to test validity for transaction chains (e.g. with multiple spending paths and where you don’t want to broadcast yet); closes #18480.
It’s also a first step towards package validation in a minimally invasive way.
The RPC commit happens to close #21074 by clarifying the “allowed” key.

There are a few added restrictions on the packages, mostly to simplify the logic for areas that aren’t critical to main package use cases:

No package can have conflicts, i.e. none of them can spend the same inputs, even if it would be a valid BIP125 replacement.
The package cannot conflict with the mempool, i.e. RBF is disabled.
The total count of the package cannot exceed 25 (the default descendant count limit), and total size cannot exceed 101KvB (the default descendant size limit).

If you’re looking for review comments and github isn’t loading them, I have a gist compiling some topics of discussion here

DrahtBot added the label Mempool on Jan 3, 2021

DrahtBot added the label P2P on Jan 3, 2021

DrahtBot added the label RPC/REST/ZMQ on Jan 3, 2021

DrahtBot added the label Validation on Jan 3, 2021

MarcoFalke commented at 9:20 am on January 3, 2021: member

Thanks for working on this. Strong Concept ACK!

jnewbery commented at 10:38 am on January 3, 2021: member

Concept ACK and approach ACK.

DrahtBot commented at 11:29 am on January 3, 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:

#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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:1075 in 25412b8d13 outdated

1077+        if (!PolicyScriptChecks(args, results[i], workspaces[i], txdata)) {
1078+            results[i].m_accepted = false;
1079+            return std::vector<MempoolAcceptResult> {std::move(results[i])};
1080+        }
1081+
1082+        if (!ConsensusScriptChecks(args, results[i], workspaces[i], txdata)) {

sdaftuar commented at 7:57 pm on January 5, 2021:

This means that we will update our script cache with transactions that might not ultimately get accepted to the mempool? This would need to be restructured before exposing to the p2p network to avoid becoming a DoS vector.

sdaftuar commented at 8:12 pm on January 5, 2021:

On further thought: if you’re only implementing this for testmempoolaccept anyway, why bother with the call to ConsensusScriptChecks? It can only return failure if our software is broken – it’s a safeguard against miners creating invalid blocks, not something that users should expect to ever run into.

If you drop this call, then dropping CIFMAC is also no longer necessary, right?

glozow commented at 8:37 pm on January 5, 2021:

AFAIK we only use the script cache when it passes, so I don’t think it’s a DoS vector? On master, if you call testmempoolaccept, it caches the successful script executions with consensus flags.

On further thought: if you’re only implementing this for testmempoolaccept anyway, why bother with the call to ConsensusScriptChecks?

Good point, PolicyScriptChecks is stricter so ConsensusScriptChecks is unnecessary. And yes, we could then keep CIFMAC. But in order to do actual, non-test-accepts, we’ll have to revisit the issue again (since I assume we’ll want to call ConsensusScriptChecks then). It’d be “kicking the can down the road.”

sdaftuar commented at 2:53 pm on January 6, 2021:

I assume that the ultimate goal of package validation is to only accept the whole set of transactions if each one individually makes it into the mempool (probably along with some other properties on the feerate of the package being used rather than individual feerates). If that’s correct, then calling ConsensusScriptChecks on anything (which will add to the script cache) before you finish calling PolicyScriptChecks on later transactions in the package will mean that a policy failure in a later transaction could cause the whole package to be rejected, while still having updated the cache.

If exposed on the p2p network, an attacker could wipe out a target’s script cache for free this way, which is the DoS vector I was referring to.

So instead, you can make this loop just call PolicyScriptChecks, which is good enough for test_accept. Once you implement adding to the mempool on success, you could have a new loop over the set of transactions in the package (topologically sorted) that invokes ConsensusScriptChecks and then adds the transaction to the mempool. This way, CIFMAC doesn’t need to be touched at all, because all a transaction’s inputs are already in the utxo set or the mempool at the time ConsensusScriptChecks is invoked. (This is essentially the structure I proposed in #16401.)

glozow commented at 7:53 pm on January 6, 2021:

I assume that the ultimate goal of package validation is to only accept the whole set of transactions if each one individually makes it into the mempool… If that’s correct, then calling ConsensusScriptChecks on anything (which will add to the script cache) before you finish calling PolicyScriptChecks on later transactions in the package will mean that a policy failure in a later transaction could cause the whole package to be rejected, while still having updated the cache.

Right, I have the same vision in mind, and absolutely agree. I should change these two be separate loops - all PolicyScriptChecks, then all ConsensusScriptChecks. This would mean that we would only be calling ConsensusScriptChecks (which is effectively just used to cache script results) if all of them would pass - there’s no way for any of them to fail consensus if they passed policy.

glozow commented at 10:31 pm on January 6, 2021:

The more I think about this, the more it makes sense 😅 In actual package accept, we don’t need CoinsViewMempool after PolicyScriptChecks, we can actually just go one by one in the package (after topological sort) and run ConsensusScriptChecks + submit to mempool. And subsequent txns should have all of the Coins they need from the pool. Apologies if this was obvious to you - is this what you had in mind?

sdaftuar commented at 2:10 pm on January 7, 2021:

Yes – sounds like we’re on the same page!

in src/rpc/rawtransaction.cpp:891 in 10860308da outdated

887@@ -889,7 +888,7 @@ static RPCHelpMan testmempoolaccept()
888                 },
889                 RPCResult{
890                     RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n"
891-                        "Length is exactly one for now.",
892+                        "Length is exactly one if any failures occur.",

luke-jr commented at 5:05 pm on January 6, 2021:

Do we want to commit to that?

glozow commented at 10:49 pm on January 6, 2021:

I believe we plan to validate packages atomically, so we would never have a situation of “these txns passed, but those didn’t.” That’s why I believed length 1 for failure would be alright, especially since this wouldn’t be an API-breaking change. I imagine it could be helpful to return more information to the client, but don’t know what that would look like concretely.

glozow commented at 11:14 pm on January 6, 2021:

(Open to ideas)

in src/rpc/rawtransaction.cpp:924 in 10860308da outdated

930-    }
931-    CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
932-    const uint256& tx_hash = tx->GetHash();
933-
934+    UniValue transactions = request.params[0].get_array();
935+    const size_t num_txns = {transactions.size()};

luke-jr commented at 5:06 pm on January 6, 2021:

This syntax seems strange?

glozow force-pushed on Jan 6, 2021

glozow force-pushed on Jan 8, 2021

glozow marked this as ready for review on Jan 8, 2021

glozow commented at 11:10 pm on January 8, 2021: member

5th time’s the charm I guess 😂 this is ready for review!

darosior commented at 7:48 pm on January 9, 2021: member

Concept ACK :)

DrahtBot commented at 11:45 am on January 13, 2021: member

🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

in src/validation.h:201 in c97fae1011 outdated

202-                        bool bypass_limits, bool test_accept=false, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
203+/** Per-transaction result from trying to accept a transaction to the memory pool. */
204+struct MempoolAcceptResult {
205+    MempoolAcceptResult(const CTransactionRef& ptx) : txid(ptx->GetHash()) {}
206+
207+    uint256 txid;

MarcoFalke commented at 8:11 am on January 14, 2021:

in commit c97fae1011c4bd376898af73576756f163eeaf58:

This member seems currently unused? Also, what is the point of copying the (already cached) txid once more? If you need a reference to the tx, maybe store the CTransactionRef, but that seems redundant, because the caller is already aware of the tx.

glozow commented at 4:35 pm on January 14, 2021:

Right, it’s not used in https://github.com/bitcoin/bitcoin/commit/c97fae1011c4bd376898af73576756f163eeaf58. I ended up needing it for multi-accept on the failure case to indicate which tx failed. It might fit better in https://github.com/bitcoin/bitcoin/pull/20833/commits/2f36f0743158817635a39f7efb60541fe0c4b31d

glozow commented at 9:32 pm on January 19, 2021:

Moved it to the relevant commit. Putting a CTransactionRef in the MempoolAcceptResult when returning failure so that we know which tx failed; we need it to get the txid and wtxid in testmempoolaccept.

in src/validation.h:205 in c97fae1011 outdated

206+
207+    uint256 txid;
208+    bool m_accepted = false;
209+    TxValidationState m_state;
210+    std::list<CTransactionRef> m_replaced_transactions{};
211+    CAmount m_fee = CAmount(0);

MarcoFalke commented at 8:12 am on January 14, 2021:

Would be nice to clarify what kind of fee this is. Probably base fee and not prioritized fee?

MarcoFalke commented at 8:26 am on January 14, 2021:

Not sure if it makes sense to return a fee of 0 when the tx was rejected. At the very least the members that are optional, should be std::optional. Though, I am thinking that it could make sense to return two completely different types, based on whether the tx was accepted? I.e. a struct with {fee, replaced_txs} if the tx was accepted and a struct with {state} if the tx was rejected.

This would make it harder at the call sites to make mistakes such as returning the fee of a rejected tx.

glozow commented at 6:47 pm on January 14, 2021:

Ah true! I’m also starting to think that m_accepted == m_state.IsValid() always?? 🤔 maybe could get rid of that and do

0TxValidationState m_state;
1std::optional<std::list<CTransactionRef>> m_replaced_transactions;
2std::optional<CAmount> m_base_fee;

MarcoFalke commented at 7:10 pm on January 14, 2021:

Not sure about the equivalence right now, but if m_accepted then m_state.IsValid() should hold.

So you don’t need to return a validation state if the tx was accepted. Though, instead of having a return struct with

0optional<state>
1optional<fee>
2optional<txs>
3...

It could make sense to have one struct Success with

0fee
1txs

and then return std::variant<Sucess, TxValidationState>

wdyt?

glozow commented at 5:45 pm on January 18, 2021:

Yeah I agree - I just started writing this and I think we should actually put the TxValidationState and m_base_fees into the MemPoolAccept::Workspace (since we need access throughout validation), then return whichever is appropriate at the end.

How does this look?

0
1struct MempoolAcceptSuccess {
2    std::list<CTransactionRef> m_replaced_transactions;
3    CAmount m_base_fee;
4}
5
6using MempoolAcceptResult = std::variant<MempoolAcceptSuccess, TxValidationState>;
7using PackageAcceptResult = std::variant<std::vector<MempoolAcceptSuccess>, std::tuple<uint256, TxValidationState>>;

MarcoFalke commented at 6:43 pm on January 18, 2021:

Looks good

glozow commented at 4:51 pm on January 19, 2021:

ay @MarcoFalke, the std::variant method is a bit hairy… see this branch. It works, but has a larger diff so I’m worried it might encumber reviewers. If you think it’s an improvement, I can leave it for a followup or something?

glozow commented at 9:30 pm on January 19, 2021:

taken!

MarcoFalke commented at 8:26 am on January 14, 2021: member

left some nits in the first commit

DrahtBot added the label Needs rebase on Jan 14, 2021

in test/functional/rpc_packages.py:25 in 79e5512206 outdated

20+)
21+
22+class RPCPackagesTest(BitcoinTestFramework):
23+    def set_test_params(self):
24+        self.num_nodes = 1
25+        self.extra_args = [[]]

mjdietzx commented at 5:19 pm on January 14, 2021:

nit: you can probably ditchself.extra_args = [[]] right?

glozow commented at 6:49 pm on January 14, 2021:

ya i think you’re right

in test/functional/mempool_accept.py:70 in 79e5512206 outdated

64@@ -65,7 +65,6 @@ def run_test(self):
65 
66         self.log.info('Should not accept garbage to testmempoolaccept')
67         assert_raises_rpc_error(-3, 'Expected type array, got string', lambda: node.testmempoolaccept(rawtxs='ff00baar'))
68-        assert_raises_rpc_error(-8, 'Array must contain exactly one raw transaction for now', lambda: node.testmempoolaccept(rawtxs=['ff00baar', 'ff22']))

mjdietzx commented at 5:21 pm on January 14, 2021:

would it be better to check that this now passes without throwing rather than removing it?

glozow commented at 6:24 pm on January 14, 2021:

I preferred keeping the package-related tests in rpc_package.py

glozow commented at 6:25 pm on January 14, 2021:

(also these 2 are not well-formed transactions, so it would return a deserialization error)

mjdietzx commented at 6:29 pm on January 14, 2021:

OK, that makes sense. All good 👍

mjdietzx commented at 6:13 pm on January 14, 2021: contributor

So far I’ve reviewed the functional and unit tests and they look great! Well done doing that in a way that doesn’t require the wallet - very thorough

I think soon test/functional/rpc_packages.py can be done with MiniWallet and get rid of some code. It would probably need #20889 though bc we’d have to catch some exceptions where we expect from_node.testmempoolaccept to fail. Anyways I wouldn’t expect that to be done in this PR or delay anything, tests look great as is

glozow commented at 6:45 pm on January 14, 2021: member

@mjdietzx thanks for review! :)

I think soon test/functional/rpc_packages.py can be done with MiniWallet and get rid of some code.

I didn’t feel like MiniWallet suited my needs at the moment - I needed to chain transactions and wanted more control over the scripts. I considered just adding more functionality to MiniWallet but it might not be needed elsewhere and would conflict with a lot of the PRs that stem from #20078 (which I am a huge fan of) 😃. Certainly interested in putting more of the code in test_framework/wallet.py in the future, if it could be reused!

in src/bench/block_assemble.cpp:51 in c97fae1011 outdated

47@@ -48,9 +48,8 @@ static void AssembleBlock(benchmark::Bench& bench)
48         LOCK(::cs_main); // Required for ::AcceptToMemoryPool.
49 
50         for (const auto& txr : txs) {
51-            TxValidationState state;
52-            bool ret{::AcceptToMemoryPool(*test_setup.m_node.mempool, state, txr, nullptr /* plTxnReplaced */, false /* bypass_limits */)};
53-            assert(ret);
54+            const MempoolAcceptResult res = ::AcceptToMemoryPool(*test_setup.m_node.mempool, txr, false /* bypass_limits */);

ariard commented at 0:39 am on January 18, 2021:

I don’t think we have a coding style recommendation for this, but IMO it’s more intuitive to have comment - arg rather than arg - comment. At least other codebase callsites are following this.

glozow commented at 9:29 pm on January 19, 2021:

I agree, really, but didn’t change it because I’d rather put all my opinion-energy into having a MempoolAcceptResult struct 😛

in src/node/transaction.cpp:57 in c97fae1011 outdated

58-            if (!AcceptToMemoryPool(*node.mempool, state, tx,
59-                nullptr /* plTxnReplaced */, false /* bypass_limits */, /* test_accept */ true, &fee)) {
60-                return HandleATMPError(state, err_string);
61-            } else if (fee > max_tx_fee) {
62+            const MempoolAcceptResult result =
63+                AcceptToMemoryPool(*node.mempool, tx, false /* bypass_limits */, true /* test_accept */);

ariard commented at 0:42 am on January 18, 2021:

Why breaking line for rvalue ?

glozow commented at 9:25 pm on January 19, 2021:

I don’t like long lines 🤷 I put it back since it’s not too bad

in src/validation.h:197 in c97fae1011 outdated

198- * plTxnReplaced will be appended to with all transactions replaced from mempool
199- * @param[out] fee_out optional argument to return tx fee to the caller **/
200-bool AcceptToMemoryPool(CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx,
201-                        std::list<CTransactionRef>* plTxnReplaced,
202-                        bool bypass_limits, bool test_accept=false, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
203+/** Per-transaction result from trying to accept a transaction to the memory pool. */

ariard commented at 1:07 am on January 18, 2021:

“Evaluation result of a single-transaction mempool acceptance”, better than repeating twice transaction ?

in src/validation.cpp:684 in 4a8e3e1a17 outdated

658@@ -659,11 +659,6 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, MempoolAcceptResult& result, Works
659     // Bring the best block into scope
660     m_view.GetBestBlock();
661 
662-    // we have all inputs cached now, so switch back to dummy (to protect
663-    // against bugs where we pull more inputs from disk that miss being added
664-    // to coins_to_uncache)
665-    m_view.SetBackend(m_dummy);

ariard commented at 2:34 am on January 18, 2021:

I need to dig more but if we do have a bug slips in, you might inflate the utxo cache and not update accurately coins_to_uncache. Thus in case of invalid transactions it avoids wasting cache space with junks. I don’t understand your commit rational to deprecate this belt-and-suspender.

glozow commented at 2:51 pm on January 18, 2021:

Let me try to elaborate a bit: when validating packages, we have each coin in the m_viewmempool, which at this point is the m_view backend. If we set the backend to dummy, we lose the ability to look up those coins. Typically, the backend allows for going to disk (which would be unnecessary after this line and be the source of a coins_to_uncache leak). Since we need the coins in CheckSequenceLocks right afterward, and then for CheckInputScripts again later, I got rid of it 🤔 maybe a little aggressive oops

The lines above for each input:

0        if (!coins_cache.HaveCoinInCache(txin.prevout)) {
1            coins_to_uncache.push_back(txin.prevout);
2        }

should catch all of the coins that we need to uncache. It’s a belt-and-suspenders check I would prefer not to remove, but we would need to implement a little differently. Perhaps a boolean in CCoinsViewMemPool for allowing fetching from disk?

glozow commented at 3:16 pm on January 27, 2021:

@ariard, thanks for the push to look into this. I got a pretty simple solution: we shouldn’t do m_view.SetBackend(dummy), we should do m_viewmempool.SetBackend(dummy), since that’s the backend that’s actually pointing to the coins cache.

The hierarchy is m_view -> m_viewmempool -> coinscache.

in src/validation.cpp:608 in 2f36f07431 outdated

604@@ -602,6 +605,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, MempoolAcceptResult& result, Works
605         return state.Invalid(TxValidationResult::TX_CONFLICT, "txn-already-in-mempool");
606     }
607 
608+    // Check for duplicates in package

ariard commented at 2:38 am on January 18, 2021:

I don’t think you need mempool state for this new check and the other one. You’re verifying package element against each other (txid-vs-txid, input-vs-input).

Maybe they could be gathered in some SanitizePackage function called in ProcessNewPackage. This would avoid encumbering more PreChecks() code path.

glozow commented at 9:27 pm on January 19, 2021:

Right good point. I’ve removed it since checking inputs is a better way to test duplicates/conflicts. I’ll note for the future to do context-free sanitization checks before taking the lock :)

in src/rpc/rawtransaction.cpp:950 in 2f36f07431 outdated

975+                AcceptToMemoryPool(mempool, txns[0], false /* bypass_limits */, true /* test_accept */));
976+        }
977+    } else {
978+        {
979+            LOCK(cs_main);
980+            validation_results = ProcessNewPackage(mempool, txns, true);

ariard commented at 2:41 am on January 18, 2021:

I think you can split ProcessNewPackage()/AcceptMultipleTransactions in its own commit. Better to isolate mempool changes from rpc-level ones IMO.

glozow commented at 9:27 pm on January 19, 2021:

taken

ariard commented at 3:21 am on January 18, 2021: member

I think this PR needs to be better motivated. I believe the gradual package validation introduction is a good idea but I do have questions around the following rational:

This allows you to test validity for second-layer application transaction chains (i.e. with multiple spending paths and where you don’t want to broadcast yet).

I’ve a hard time envisioning a deployed/soon-to-be-deployed L2 protocol security model where it does make sense to test mempool validity of a transaction chain as this PR allows.

If you don’t have the need to broadcast your chain of transactions, its feerate might not be finalized and thus minimal, under your current mempool feerate. Or if second-stage transactions are malleable (anchor output format in LN) you might aggregate them to reduce the fee cost at broadcast and thus rendering the previous mempool evaluation irrelevant.

Or the second-stage of your chain might be under an absolute timelocks according to protocol transaction format (e.g HTLC-timeout) and as such will be also rejected by testmempoolaccept but it’s valid according to protocol semantics.

Further, if the second-stage of your chain are constrained by an off-chain covenant (a.k,a double-signed by both protocol parties), forcing to produce the local signature for testing mempool validity isn’t the best security advice.

In most L2s, chain of transactions validity is more liberal than mempool one. Integrating this new testmempoolaccept might break it… @darosior do you intend to use this for a vault stack ? If yes that would be great if could you describe the interaction between this new testmempoolaccept and your verification flow!

darosior commented at 8:34 am on January 18, 2021: member

@ariard i think it is only intended to be a convenient way to sanity check for “static” standardness bounds, not for testing the whole validity prior to broadcast (which is intractable..).

ariard commented at 2:05 am on January 19, 2021: member

@darosior Yes but as testmempoolaccept result is {success, failure} so for e.g if a package tx is time-locked in the future, you will get a failure and wrongly fail your L2 flow ? I don’t think “static” (or stateless?) standardness bounds is more defined.

darosior commented at 11:54 am on January 19, 2021: member

@ariard i think it would be useful to test the first stage -non timelocked- flow (in specific for Revault, the deposit-emergency, deposit-unvault-cancel, and deposit-unvault-emergency scenarii) without mocking actual tx sending and block generation. I don’t think it’d be really useful for “check at runtime” either. As for the “static” bounds, there are for instance the maximum standard witscript size or minimum relay fee that are time (and fee-bumping) independent.

glozow force-pushed on Jan 19, 2021

glozow commented at 9:59 pm on January 19, 2021: member

Rebased, split up the refactoring commit (hopefully now very digestible chunks), and took some of the review suggestions so far. Thanks a ton for your reviews @jnewbery @MarcoFalke @sdaftuar @mjdietzx @darosior @ariard 🤗 hope you take a look again! @ariard Re: timelocks, off-chain covenants, etc. yep this obviously has some limitations and I wouldn’t consider this a perfect tester for L2 transactions. If we could come up with some test vectors where, for example, the option to ignore timelocks in dry-runs would be extremely useful, I think we could consider it. Regardless, I think a basic package accept like this moves in that direction :)

DrahtBot removed the label Needs rebase on Jan 19, 2021

glozow force-pushed on Jan 20, 2021

in src/validation.h:204 in c75f705292 outdated

202+    bool m_accepted;
203+    TxValidationState m_state;
204+
205+    // Valid when m_accepted = true
206+    std::list<CTransactionRef> m_replaced_transactions;
207+    CAmount m_base_fees;

MarcoFalke commented at 4:56 pm on January 21, 2021:

Unless I am mistaken, incorrectly reading this will result in an uninitialized read, which is only detected by valgrind. Wouldn’t it be better to make this safe for non-valgrind use via an std::optional at least. Or is that also a bit too verbose (like the std::variant approach)?

glozow commented at 5:14 pm on January 22, 2021:

Yeah good point, let me whip up the std::optional method real quick and get back to you on the verbosity

glozow commented at 3:32 pm on January 25, 2021:

Please see this branch, it’s not too bad verbosity-wise, I have a couple ternary operators like this:

0const TxValidationState state = result.m_state == nullopt ? TxValidationState{} : result.m_state.value();

incorrect read throws a bad optional access

MarcoFalke commented at 3:48 pm on January 25, 2021:

Cool, thanks for looking. Not sure if the ternary makes sense. It seems state is currently assumed to be always properly initialized (redundantly to the boolean return value). So maybe it could make sense to not make this optional. I do like that the other members (fee and replaced txs) are optional in your branch.

glozow commented at 4:29 pm on January 25, 2021:

Ah yeah! That makes sense, leave state as-is, and optional members when they only make sense for valid transactions.

ariard commented at 1:45 am on January 22, 2021: member

@glozow @darosior I think we need distinctions to understand better what this package-testmempoolaccept is achieving for transaction chains. I agree that it lets L2 application devs manually test the mempool validity of a static chain of transactions, and that way avoid to deploy a completely broken stack in production. Just note the caveat, fRequireStandard is false for testnet, so be careful to do dry-run in regtest.

But beyond, L2s may have dynamic chain of transactions, i.e transactions function of counterparty contributions (e.g LN update_add_htlc) or behaviors (e.g a remote commitment broadcast). Asserting mempool validity of this type of chain is irrelevant as soon as you start to be in production, a malicious counterparty might hit a standard bound you forgot or weren’t able to test during dry-runs. As a L2 dev what you’re aiming for is a txstandardness verifier, a tool encapsulating ATMP checks. Maybe with some configurable options like fRelaxTimelocks or fBypassFees to morph the checks to your protocol flow.

My final point, we should be careful to scope this new testmempoolaccept to the first usage in the release notes. Just to avoid someone hoping to achieve the second usage after this change and potentially breaking stuff on dumb-or-edge cases :)

I’ll be back to code review soon.

glozow force-pushed on Jan 22, 2021

glozow commented at 5:26 pm on January 22, 2021: member

@ariard

Just note the caveat, fRequireStandard is false for testnet, so be careful to do dry-run in regtest.

I think fRequireStandard is true for regtest and false for testnet.

My final point, we should be careful to scope this new testmempoolaccept to the first usage in the release notes. Just to avoid someone hoping to achieve the second usage after this change and potentially breaking stuff on dumb-or-edge cases :)

I certainly hope it didn’t seem like I was over-promising what this enables, agree about clearly defining the (limited) scope. I’ll be sure to tag you for release notes review :)

in test/functional/rpc_packages.py:155 in ebca80d811 outdated

102+        for _ in range(3):
103+            value -= Decimal("0.0001") # Deduct reasonable fee
104+            (txid, txhex, scriptPubKey) = self.chain_transaction(txid, value, scriptPubKey)
105+            chain.append(txhex)
106+
107+        self.log.info("Testmempoolaccept with entire package")

robot-dreams commented at 1:04 pm on January 23, 2021:

Would it be useful to add a short check here that testmempoolaccept fails if the transactions aren’t sorted? For example:

0        self.log.info("Test package with unsorted transactions isn't accepted")
1        unsorted_chain = [chain[0], chain[2], chain[1]]
2        orphaned = CTransaction()
3        orphaned.deserialize(BytesIO(hex_str_to_bytes(unsorted_chain[1])))
4        testres_bad = node.testmempoolaccept(rawtxs=unsorted_chain)
5        assert_equal(testres_bad, [{"txid": orphaned.rehash(), "wtxid": orphaned.getwtxid(), "allowed": False, "reject-reason": "missing-inputs"}])

glozow commented at 3:33 pm on January 25, 2021:

good point! will add that test

in src/rpc/rawtransaction.cpp:906 in ebca80d811 outdated

887@@ -889,7 +888,7 @@ static RPCHelpMan testmempoolaccept()
888                 },
889                 RPCResult{
890                     RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n"

robot-dreams commented at 1:11 pm on January 23, 2021:

Nit: Is it worth explicitly mentioning that on failure, the one entry in the result array corresponds to the first transaction in the package that failed validation?

glozow commented at 3:38 pm on January 27, 2021:

Done!

in src/txmempool.h:891 in ebca80d811 outdated

879@@ -880,15 +880,28 @@ class CTxMemPool
880  * It also allows you to sign a double-spend directly in
881  * signrawtransactionwithkey and signrawtransactionwithwallet,
882  * as long as the conflicting transaction is not yet confirmed.
883+ * It can also serve as temporary scratch space for some set

robot-dreams commented at 1:26 pm on January 23, 2021:

Style nit (feel free to ignore):

Rather than extending CCoinsViewMemPool, would it make sense to instead add something like CCoinsViewPackage : CCoinsViewBacked? Doing so might have the following advantages:

The backing CCoinsView doesn’t have to be a CCoinsViewMemPool (e.g. what if you want to submit a package to a block-relay-only node in the future)
It separates the concerns of tracking mempool coins and having scratch space
It makes it more obvious that each time you validate a package, you need a separate instance of CCoinsViewPackage (and that you shouldn’t reuse an existing CCoinsViewMemPool)

glozow commented at 7:57 pm on January 25, 2021:

Rather than extending CCoinsViewMemPool, would it make sense to instead add something like CCoinsViewPackage : CCoinsViewBacked?

This is a really interesting idea, and I think it should be the case to have the “top level” have the package caches -> “bottom level” backend is the CCoinsViewMemPool. Let me try to do it this way and see what happens!

glozow commented at 7:59 pm on January 25, 2021:

Note that CCoinsViewMemPool is created with MemPoolAccept instances so we don’t really reuse an existing one.

in src/validation.h:226 in ebca80d811 outdated

227+                        bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
228+
229+/**
230+* Atomically test acceptance of multiple transactions.
231+* If validation fails for any individual transaction, this returns
232+* a single MempoolAcceptResult with the failure. If all successful,

robot-dreams commented at 1:57 pm on January 23, 2021:

Nit: Is it worth saying “with the first failure”?

in src/validation.cpp:5071 in ebca80d811 outdated

5070@@ -5011,10 +5071,10 @@ bool LoadMempool(CTxMemPool& pool)
5071             TxValidationState state;

robot-dreams commented at 2:04 pm on January 23, 2021:

Nit: Is state still needed here?

glozow commented at 3:37 pm on January 27, 2021:

good catch, removed 👍 (btw, how did you comment on a non-diff line? 😱 )

in src/rpc/rawtransaction.cpp:939 in ebca80d811 outdated

954-        test_accept_res = AcceptToMemoryPool(mempool, state, std::move(tx),
955-            nullptr /* plTxnReplaced */, false /* bypass_limits */, /* test_accept */ true, &fee);
956+    for (unsigned int i = 0; i < raw_transactions.size(); ++i) {
957+        CMutableTransaction mtx;
958+        if (!DecodeHexTx(mtx, raw_transactions[i].get_str())) {
959+            throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed");

robot-dreams commented at 2:15 pm on January 23, 2021:

Is it worth keeping some flavor of the “Make sure the tx has at least one input.” message that was removed above?

in src/rpc/rawtransaction.cpp:961 in ebca80d811 outdated

 995+
 996+    // TODO: Report absurd fees for packages.
 997+
 998+    UniValue result(UniValue::VARR);
 999+    if (all_valid) {
1000+        for (unsigned int i = 0; i < validation_results.size(); ++i) {

robot-dreams commented at 2:40 pm on January 23, 2021:

Just confirming, does the requirement “either return all successes or just the first failure” apply to the RPC as a whole, or only to the internal call ProcessNewPackage?

I ask because it looks like you could add rejected transactions inside this loop. For example, if I update test_chain in rpc_packages.py so that the 2nd transaction in the chain has an unreasonable fee, the RPC response testres_multiple is as follows:

0[
1{'txid': ..., 'wtxid': ..., 'allowed': True, 'vsize': 191, 'fees': {'base': Decimal('0.00010000')}},
2{'txid': ..., 'wtxid': ..., 'allowed': False, 'reject-reason': 'max-fee-exceeded'},
3{'txid': ..., 'wtxid': ..., 'allowed': True, 'vsize': 191, 'fees': {'base': Decimal('0.00010000')}}
4]

glozow commented at 4:39 pm on January 25, 2021:

it applies to the RPC as well, only 1 result on failure

robot-dreams commented at 2:59 pm on January 23, 2021: contributor

Strong Concept ACK!

in src/validation.h:201 in ebca80d811 outdated

200+*/
201+struct MempoolAcceptResult {
202+    bool m_accepted;
203+    CTransactionRef ptx;
204+
205+    // Valid when m_accepted = false

MarcoFalke commented at 3:50 pm on January 25, 2021:

Note that m_state.IsValid() holds when m_accpeted = true, so this comment could be confusing. I think the comment can just be removed.

glozow commented at 3:42 pm on January 27, 2021:

I now have the comment “Valid when m_accepted = false” for the tx pointer, not for the TxValidationState

glozow force-pushed on Jan 27, 2021

in src/rpc/rawtransaction.cpp:955 in 70f84b2219 outdated

992-                result_0.pushKV("reject-reason", "missing-inputs");
993+    auto tx_accepted = [](MempoolAcceptResult& res) { return res.m_accepted; };
994+    const bool all_valid = std::all_of(validation_results.begin(), validation_results.end(), tx_accepted);
995+    // ProcessNewPackage should return a MempoolAcceptResult per transaction
996+    // or exactly 1 for the first error that occurs.
997+    CHECK_NONFATAL(all_valid || validation_results.size() == 1);

satsie commented at 4:19 pm on January 27, 2021:

I’m not very familiar with CHECK_NONFATAL but see it throws an error if either condition here evaluates to false.

If validation_results.size() == 1 that means one of the transactions failed validation, so we want to throw an error. Should this line be changed to CHECK_NONFATAL(all_valid || validation_results.size() != 1);?

glozow commented at 4:48 pm on January 27, 2021:

CHECK_NONFATAL (documented in developer notes here) helps with internal logic bugs. Here, the bug would be a testmempoolaccept thing only (and therefore not be fatal enough to crash the node), but we’d definitely want to address it.

Here, we are merely making sure that, if there were any errors, the length of validation_results should be 1. It’s okay for a transaction to be invalid so we don’t want to throw an error for that. This check is just to make sure that ProcessNewPackage() is adhering to the API we expect. I hope this helps!

satsie commented at 7:21 pm on January 27, 2021:

Ah! Thanks for linking to that description 👍 This was the only one I had been working off of so the link you provided is super helpful for understanding the purpose of CHECK_NONFATAL.

I was getting hung up on the ||, or part of the evaluation. I think I understand it now, an error will only be thrown if both all_valid and validation_results.size() == 1 evaluate to false, reason being that these two statements are incompatible (if there is an invalid tx, validation_results.size() can’t be anything other than 1). Thanks for the clarification! Please mark this guy as resolved when you get a chance! (I don’t believe I have permission to)

in src/validation.cpp:392 in 70f84b2219 outdated

391-        if (!fAddToMempool || (*it)->IsCoinBase() ||
392-            !AcceptToMemoryPool(mempool, stateDummy, *it,
393-                                nullptr /* plTxnReplaced */, true /* bypass_limits */)) {
394+        const MempoolAcceptResult result = AcceptToMemoryPool(mempool, *it, true /* bypass_limits */);
395+
396+        if (!fAddToMempool || (*it)->IsCoinBase() || !result.m_accepted) {

jnewbery commented at 4:26 pm on January 27, 2021:

This is a change in behavior. Previously, if fAddToMempool is false or (*it)->IsCoinBase(), we’d short circuit and not call AcceptToMemoryPool()

glozow commented at 4:10 am on January 28, 2021:

ah, right

in src/validation.h:202 in 70f84b2219 outdated

203+struct MempoolAcceptResult {
204+    bool m_accepted;
205+    TxValidationState m_state;
206+
207+    // Valid when m_accepted = false
208+    std::optional<CTransactionRef> m_ptx;

jnewbery commented at 4:28 pm on January 27, 2021:

Is it necessary to wrap the std::shared_ptr in a std::optional? Can you just use a null pointer to mean not present?

glozow commented at 1:51 am on January 29, 2021:

Mm, I agree that’s better. Also renaming it to m_failed_ptx to make it more clear.

in src/validation.h:227 in 70f84b2219 outdated

228+
229+/**
230+ * (Try to) add a transaction to the memory pool.
231+ */
232+MempoolAcceptResult AcceptToMemoryPool(CTxMemPool& pool, const CTransactionRef &tx,
233+                        bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

jnewbery commented at 4:28 pm on January 27, 2021:

Maybe clean up the style while you’re here:

CTransactionRef &tx -> CTransactionRef& tx
align second line with opening parens

in src/validation.cpp:1114 in 70f84b2219 outdated

1119 /** (try to) add transaction to memory pool with a specified acceptance time **/
1120-static bool AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, TxValidationState &state, const CTransactionRef &tx,
1121-                        int64_t nAcceptTime, std::list<CTransactionRef>* plTxnReplaced,
1122-                        bool bypass_limits, bool test_accept, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1123+static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, const CTransactionRef &tx,
1124+                        int64_t nAcceptTime, bool bypass_limits, bool test_accept) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

jnewbery commented at 4:33 pm on January 27, 2021:

Try to keep lines below 100-120 chars and align args with opening parens:

0static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool,
1                                                      const CTransactionRef& tx, int64_t nAcceptTime,
2                                                      bool bypass_limits, bool test_accept)
3    EXCLUSIVE_LOCKS_REQUIRED(cs_main)

in src/validation.cpp:1068 in f14f8b7390 outdated

1076-    MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, plTxnReplaced, bypass_limits, coins_to_uncache, test_accept, fee_out };
1077-    bool res = MemPoolAccept(pool).AcceptSingleTransaction(tx, args);
1078-    if (!res) {
1079+    CAmount fee = 0;
1080+    std::list<CTransactionRef> m_replaced_transactions;
1081+    MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, m_replaced_transactions, bypass_limits, coins_to_uncache, test_accept, fee };

jnewbery commented at 4:48 pm on January 27, 2021:

In commit f14f8b73900d2da4e15119780e19683c8499b700 ([refactor] return MempoolAcceptResult), these local vars shouldn’t be necessary. You’re simply initializing them to default values, then passing them to the ATMPArgs initializer which copies them. You can save yourself some typing (and potentially an unnecessary copy) by creating those default values in the initializer itself:

 0@@ -1063,9 +1063,7 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp
 1 {
 2     TxValidationState state;
 3     std::vector<COutPoint> coins_to_uncache;
 4-    CAmount fee = 0;
 5-    std::list<CTransactionRef> m_replaced_transactions;
 6-    MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, m_replaced_transactions, bypass_limits, coins_to_uncache, test_accept, fee };
 7+    MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, {}, bypass_limits, coins_to_uncache, test_accept, {} };
 8 
 9     const MempoolAcceptResult result = MemPoolAccept(pool).AcceptSingleTransaction(tx, args);
10     if (!result.m_accepted) {

in src/validation.cpp:5087 in 70f84b2219 outdated

5085             if (nTime > nNow - nExpiryTimeout) {
5086                 LOCK(cs_main);
5087-                AcceptToMemoryPoolWithTime(chainparams, pool, state, tx, nTime,
5088-                                           nullptr /* plTxnReplaced */, false /* bypass_limits */,
5089+                const MempoolAcceptResult result = AcceptToMemoryPoolWithTime(chainparams, pool, tx, nTime,
5090+                                           false /* bypass_limits */,

jnewbery commented at 4:50 pm on January 27, 2021:

maintain alignment with parens

in src/net_processing.cpp:2181 in 70f84b2219 outdated

2176@@ -2177,10 +2177,10 @@ void PeerManagerImpl::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
2177         if (orphan_it == mapOrphanTransactions.end()) continue;
2178 
2179         const CTransactionRef porphanTx = orphan_it->second.tx;
2180-        TxValidationState state;
2181-        std::list<CTransactionRef> removed_txn;
2182+        const MempoolAcceptResult result = AcceptToMemoryPool(m_mempool, porphanTx, false /* bypass_limits */);
2183+        const TxValidationState state = result.m_state;

jnewbery commented at 4:52 pm on January 27, 2021:

This can be a reference

in src/net_processing.cpp:3187 in 70f84b2219 outdated

3182@@ -3183,10 +3183,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3183             return;
3184         }
3185 
3186-        TxValidationState state;
3187-        std::list<CTransactionRef> lRemovedTxn;
3188+        const MempoolAcceptResult result = AcceptToMemoryPool(m_mempool, ptx, false /* bypass_limits */);
3189+        const TxValidationState state = result.m_state;

jnewbery commented at 4:53 pm on January 27, 2021:

This can be a reference.

dunxen commented at 4:56 pm on January 27, 2021: contributor

Concept ACK. A great first step for packages!

in src/rpc/rawtransaction.cpp:967 in 70f84b2219 outdated

1004+            UniValue result_inner(UniValue::VOBJ);
1005+            const CTransaction tx = *txns[i];
1006+            result_inner.pushKV("txid", tx.GetHash().GetHex());
1007+            result_inner.pushKV("wtxid", tx.GetWitnessHash().GetHex());
1008+
1009+            const CAmount fee = validation_results[i].m_base_fees.value();

jnewbery commented at 7:14 pm on January 27, 2021:

I don’t think this local variable is needed. It’s only used in one place below (in the if conditional). You could just use validation_results[i].m_base_fees.value() there.

glozow commented at 1:34 am on January 29, 2021:

Also used in fees.pushKV("base", ValueFromAmount(fee)) if that’s better?

jnewbery commented at 1:57 pm on February 1, 2021:

Oops. You’re right. Ignore this!

jnewbery commented at 7:19 pm on January 27, 2021: member

I’ve reviewed the first two commits (up to f14f8b7390 [refactor] return MempoolAcceptResult)

in src/test/txvalidation_tests.cpp:39 in f14f8b7390 outdated

41-    BOOST_CHECK_EQUAL(
42-            false,
43-            AcceptToMemoryPool(*m_node.mempool, state, MakeTransactionRef(coinbaseTx),
44-                nullptr /* plTxnReplaced */,
45-                true /* bypass_limits */));
46+    BOOST_CHECK_EQUAL(false, result.m_accepted);

fjahr commented at 0:41 am on January 28, 2021:

in f14f8b73900d2da4e15119780e19683c8499b700:

nit: I slightly prefer

0BOOST_CHECK(!result.m_accepted);

glozow force-pushed on Jan 29, 2021

glozow commented at 2:14 am on January 29, 2021: member

Thanks for your review @jnewbery, addressed your comments and some other style stuff in the last push!

stackman27 commented at 5:59 pm on January 29, 2021: contributor

Reviewed Functional tests and everything looks very good. I just had a hanging question, is there a limit to the number of rawtx a user can send in a package testmempoolaccept? or is the limit even necessary?

glozow commented at 8:08 pm on January 29, 2021: member

@stackman27 that’s a good point, no package limits could be a bit of a footgun even in testmempoolaccept. Maybe limit 25, which would be the same as DEFAULT_DESCENDANT_LIMIT? Mempool wouldn’t consider packages larger than that anyway, and if it’s not 1 package then they don’t need to put it in one call.

in src/validation.h:199 in b76910ebd4 outdated

203+struct MempoolAcceptResult {
204+    bool m_accepted;
205+    TxValidationState m_state;
206+
207+    // Valid when m_accepted = true
208+    std::optional<std::list<CTransactionRef>> m_replaced_transactions;

fjahr commented at 9:25 am on January 30, 2021:

in b76910ebd4d8a07534e145c498c7f7d0ce297ccd:

Do these have to be optional if they are “guarded” by m_accepted anyway? Especially the list could just be empty anyway.

glozow commented at 3:48 pm on February 1, 2021:

We had a short discussion about it here. I think the main idea is to distinguish between a meaningless fee/replaced_transactions and a 0/empty one. It is “guarded” but technically the caller could still access them 😅 don’t want them to do that by accident.

in src/validation.h:198 in b76910ebd4 outdated

199+* Validation result for a single transaction mempool acceptance.
200+* When m_accepted = true, m_replaced_transactions contains a list
201+* of replaced transactions and m_base_fees contains the tx fees.
202+*/
203+struct MempoolAcceptResult {
204+    bool m_accepted;

fjahr commented at 9:33 am on January 30, 2021:

in b76910e:

Stylistically I find it odd to give public members of a struct the m_* prefix. The way we use structs usually, just bundling some data without complex methods, it doesn’t seem needed internally to the struct and externally, where the struct is used looks very wrong to me. But there doesn’t seem to be a clear rule on this looking at the codebase. I just did it this way in IndexSummary and I didn’t get complaints. So maybe something to think about.

jnewbery commented at 1:32 pm on February 1, 2021:

There’s no guidance in the style guide on this. I tried to update the style guide to include guidance in #19759, but people had very strong feelings so I backed off.

in src/validation.h:172 in b76910ebd4 outdated

198+/**
199+* Validation result for a single transaction mempool acceptance.
200+* When m_accepted = true, m_replaced_transactions contains a list
201+* of replaced transactions and m_base_fees contains the tx fees.
202+*/
203+struct MempoolAcceptResult {

fjahr commented at 9:36 am on January 30, 2021:

in b76910e:

I think I would have left m_accepted out of the struct and have AcceptToMemoryPool return an optional of MempoolAcceptResult.

MarcoFalke commented at 6:30 am on February 1, 2021:

That wouldn’t work, because the error reason couldn’t be returned?

glozow commented at 3:34 pm on February 1, 2021:

Yeah I’m mostly trying to replicate the current usage, which is:

ATMP returns a boolean. we check that first. (equivalent = m_accepted)
if the boolean is false, we care about the TxValidationState state. I’m not sure if state.IsValid() is ever true when the boolean is false, but there is a code path for it, so I don’t want to try to change it in a refactor.
If boolean true, we sometimes look at fees and stuff.

in src/rpc/rawtransaction.cpp:974 in 048ef0a7e6 outdated

 999+            const CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size);
1000+            // Check that fee does not exceed maximum fee
1001+            if (max_raw_tx_fee && fee > max_raw_tx_fee) {
1002+                result_inner.pushKV("allowed", false);
1003+                result_inner.pushKV("reject-reason", "max-fee-exceeded");
1004+                result_inner.push_back(std::move(result_inner));

fjahr commented at 10:06 am on January 30, 2021:

in 048ef0a7e62cca10c588b451c9060e7846ac398b:

This looks like an error?

glozow commented at 3:27 pm on February 1, 2021:

woah o.O nice catch. I should add a test for absurd fees in package 😅

in src/rpc/rawtransaction.cpp:948 in b76910ebd4 outdated

960-        result_0.pushKV("reject-reason", "max-fee-exceeded");
961-        result.push_back(std::move(result_0));
962-        return result;
963-    }
964-    result_0.pushKV("allowed", test_accept_res);
965+    MempoolAcceptResult accept_result = WITH_LOCK(cs_main,

fjahr commented at 5:02 pm on January 31, 2021:

in b76910ebd4d8a07534e145c498c7f7d0ce297ccd:

I guess this result can be const, too.

in src/txmempool.h:891 in 17ca2d8e72 outdated

886  */
887 class CCoinsViewMemPool : public CCoinsViewBacked
888 {
889 protected:
890     const CTxMemPool& mempool;
891+    std::map<COutPoint, Coin> cache_package_add;

fjahr commented at 5:41 pm on January 31, 2021:

in 17ca2d8e72bc5d9249e4a01a2dbc2bb362dbd9d8:

These members could get an m_* prefix. Also I think they could use some comments explaining what context they are used in. Or the added comments above could be more explicit.

jnewbery commented at 2:34 pm on February 1, 2021:

What’s the reason for having two maps here? Why not just keep one, add to it when a new coin is created and remove from it when the coin is spent?

glozow commented at 5:24 pm on February 1, 2021:

We wouldn’t be able to distinguish between a missing-inputs and a conflict-in-package. Perhaps that’s desirable, but I felt this was simpler?

jnewbery commented at 5:35 pm on February 1, 2021:

Ah, great point. Maybe add a code comment to say that?

in test/functional/rpc_packages.py:39 in 2fac013613 outdated

29+        node = self.nodes[0]
30+        self.privkeys = [node.get_deterministic_priv_key().key]
31+        self.address = node.get_deterministic_priv_key().address
32+        self.coins = []
33+        # The last 100 coinbase transactions are premature
34+        for b in node.generatetoaddress(120, self.address)[:20]:

fjahr commented at 7:26 pm on January 31, 2021:

in 2fac0136138012ae26c8e12ef77e9486ad5cd831:

Do you have to generate these and can’t use the cached chain?

glozow commented at 3:36 pm on February 1, 2021:

I want to generate to deterministic address so I don’t need to use wallet

in src/txmempool.cpp:943 in 17ca2d8e72 outdated

947+void CCoinsViewMemPool::AddPackageTransaction(const CTransactionRef& tx) {
948+    package_txids.insert(tx->GetHash());
949+    // Coins spent by this transaction
950+    for (auto input : tx->vin) {
951+        Coin spent_coin;
952+        GetCoin(input.prevout, spent_coin);

fjahr commented at 7:55 pm on January 31, 2021:

in 17ca2d8e72bc5d9249e4a01a2dbc2bb362dbd9d8:

I am probably missing something but I find it strange that this is ignoring the result of GetCoin(). So we don’t care at this point that the prevout might already be spent? Might be worth a comment.

glozow commented at 0:52 am on February 2, 2021:

You’re right, I’ll add an Assume() so we get a debug error if it’s spent.

fjahr commented at 8:03 pm on January 31, 2021: member

Concept ACK

I don’t have a full understanding of the approach yet but leaving some comments and questions.

in src/validation.h:206 in 2fac013613 outdated

223+    explicit MempoolAcceptResult(TxValidationState state, std::list<CTransactionRef>&& replaced_txns, CAmount fees) :
224+        m_state(state), m_replaced_transactions(std::move(replaced_txns)), m_base_fees(fees) {
225+        m_accepted = true;
226+        m_failed_ptx = nullptr;
227+    }
228+};

jnewbery commented at 1:48 pm on February 1, 2021:

Since these ctors are just initializing members, you could do it all in the initializer list:

0    /** Constructor for failure case */
1    explicit MempoolAcceptResult(TxValidationState state, CTransactionRef ptx) :
2        m_accepted(false), m_state(state), m_failed_ptx(ptx), m_replaced_transactions(nullopt),
3        m_base_fees(nullopt) {}
4
5    /** Constructor for success case */
6    explicit MempoolAcceptResult(TxValidationState state, std::list<CTransactionRef>&& replaced_txns, CAmount fees) :
7        m_accepted(true), m_state(state), m_failed_ptx(nullptr),
8        m_replaced_transactions(std::move(replaced_txns)), m_base_fees(fees) {}

in src/validation.cpp:5092 in 2fac013613 outdated

5092-                                           false /* test_accept */);
5093-                if (state.IsValid()) {
5094+                const MempoolAcceptResult result = AcceptToMemoryPoolWithTime(chainparams, pool, tx, nTime,
5095+                                                                                false /* bypass_limits */,
5096+                                                                                false /* test_accept */);
5097+                if (result.m_accepted) {

jnewbery commented at 1:50 pm on February 1, 2021:

Consider joining these lines:

0                if (AcceptToMemoryPoolWithTime(chainparams, pool, tx, nTime,
1                                               false /* bypass_limits */,
2                                               false /* test_accept */).m_accepted) {

in src/validation.cpp:576 in 2fac013613 outdated

572     const int64_t nAcceptTime = args.m_accept_time;
573     const bool bypass_limits = args.m_bypass_limits;
574     std::vector<COutPoint>& coins_to_uncache = args.m_coins_to_uncache;
575 
576     // Alias what we need out of ws
577+    TxValidationState &state = ws.m_state;

jnewbery commented at 2:10 pm on February 1, 2021:

0    TxValidationState& state = ws.m_state;

in src/validation.cpp:944 in 2fac013613 outdated

942+bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws, PrecomputedTransactionData& txdata)
943 {
944     const CTransaction& tx = *ws.m_ptx;
945-
946-    TxValidationState &state = args.m_state;
947+    TxValidationState &state = ws.m_state;

jnewbery commented at 2:11 pm on February 1, 2021:

0    TxValidationState& state = ws.m_state;

in src/validation.cpp:971 in 2fac013613 outdated

969 {
970     const CTransaction& tx = *ws.m_ptx;
971     const uint256& hash = ws.m_hash;
972-
973-    TxValidationState &state = args.m_state;
974+    TxValidationState &state = ws.m_state;

jnewbery commented at 2:11 pm on February 1, 2021:

0    TxValidationState& state = ws.m_state;

in src/validation.cpp:1002 in 2fac013613 outdated

 999+bool MemPoolAccept::Finalize(const ATMPArgs& args, Workspace& ws)
1000 {
1001     const CTransaction& tx = *ws.m_ptx;
1002     const uint256& hash = ws.m_hash;
1003-    TxValidationState &state = args.m_state;
1004+    TxValidationState &state = ws.m_state;

jnewbery commented at 2:11 pm on February 1, 2021:

0    TxValidationState& state = ws.m_state;

in src/validation.cpp:1049 in 2fac013613 outdated

1045+MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args)
1046 {
1047     AssertLockHeld(cs_main);
1048     LOCK(m_pool.cs); // mempool "read lock" (held through GetMainSignals().TransactionAddedToMempool())
1049 
1050     Workspace workspace(ptx);

jnewbery commented at 2:12 pm on February 1, 2021:

Consider changing this to ws to match parameter names in the other functions.

in src/validation.cpp:473 in 2fac013613 outdated

480@@ -474,11 +481,13 @@ class MemPoolAccept
481          */
482         std::vector<COutPoint>& m_coins_to_uncache;

jnewbery commented at 2:16 pm on February 1, 2021:

This seems to be the odd one out in ATMPArgs, now that all of the others are const. Any reason that this one shouldn’t live in Workspace?

glozow commented at 5:17 pm on February 1, 2021:

Yes, I don’t like it either. The lifetime of Workspace ends when MemPoolAccept::AcceptSingleTransaction() returns, but the coins in coins_to_uncache are uncached in ATMP afterward. If it’s appropriate to do coin-uncaching inside MemPoolAccept::Accept() functions, then we can move it?

jnewbery commented at 5:34 pm on February 1, 2021:

Perhaps they could be uncached in MemPoolAccept’s destructor? That’s similar to an RAII pattern where the object releases any resources as it goes out of scope.

I don’t think this needs to be done as part of this PR. Could be a follow up if it sounds like an improvement to you.

glozow commented at 0:04 am on February 2, 2021:

Yeah, maybe for a future PR 🤔 would be nice. The only complication is that we only uncache if the validation failed, so MemPoolAccept would need to know that when it’s destructing…

glozow commented at 8:59 pm on April 16, 2021:

Started on this in #21146 if anyone’s interested, resolving here because out of scope for this PR

in src/txmempool.cpp:912 in 2fac013613 outdated

904@@ -905,19 +905,48 @@ bool CTxMemPool::HasNoInputsOf(const CTransaction &tx) const
905 CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { }
906 
907 bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const {
908+    // Check to see if the inputs are spent by a tx being considered for a package.
909+    // It's possible that a package spends entries in the mempool, and this would
910+    // only be reflected in the cache_package_remove.
911+    if (auto it = cache_package_remove.find(outpoint); it != cache_package_remove.end()) {
912+        coin = it->second;

jnewbery commented at 2:21 pm on February 1, 2021:

There’s no need to set coin in this failure case. From the interface definition in CCoinsView::GetCoin:

When false is returned, coin’s value is unspecified.

in src/txmempool.cpp:911 in 2fac013613 outdated

904@@ -905,19 +905,48 @@ bool CTxMemPool::HasNoInputsOf(const CTransaction &tx) const
905 CCoinsViewMemPool::CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn) : CCoinsViewBacked(baseIn), mempool(mempoolIn) { }
906 
907 bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const {
908+    // Check to see if the inputs are spent by a tx being considered for a package.
909+    // It's possible that a package spends entries in the mempool, and this would
910+    // only be reflected in the cache_package_remove.
911+    if (auto it = cache_package_remove.find(outpoint); it != cache_package_remove.end()) {

jnewbery commented at 2:23 pm on February 1, 2021:

You can just use std::map.count() here since you don’t actually need the value of the coin.

in src/txmempool.cpp:924 in 2fac013613 outdated

921+    }
922+
923     // If an entry in the mempool exists, always return that one, as it's guaranteed to never
924-    // conflict with the underlying cache, and it cannot have pruned entries (as it contains full)
925-    // transactions. First checking the underlying cache risks returning a pruned entry instead.
926+    // conflict with the underlying cache, and it cannot have spent entries.

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

Remove “, and it cannot have spent entries”. That’s vestigial from when a CCoins objects was returned for the full transaction.

in src/txmempool.cpp:945 in 2fac013613 outdated

940+    if (outpoint.n < ptx->vout.size()) {
941+        coin = Coin(ptx->vout[outpoint.n], MEMPOOL_HEIGHT, false);
942+        return true;
943+    }
944+    return false;
945+}

jnewbery commented at 2:30 pm on February 1, 2021:

Switching the logic ordering here is a little confusing. I’d suggest leaving it as it was.

in src/txmempool.cpp:948 in 2fac013613 outdated

952+        GetCoin(input.prevout, spent_coin);
953+        cache_package_remove.emplace(input.prevout, spent_coin);
954+    }
955+    // Coins added by this transaction
956+    for (unsigned int i = 0; i < tx->vout.size(); ++i) {
957+        cache_package_add.emplace(COutPoint(tx->GetHash(), i), Coin(tx->vout[i], MEMPOOL_HEIGHT, tx->IsCoinBase()));

jnewbery commented at 2:30 pm on February 1, 2021:

Presumably tx->IsCoinBase() will always be false here? We can’t accept coinbase transactions into our mempool.

in src/txmempool.h:901 in 2fac013613 outdated

897-    bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
898+    bool GetCoin(const COutPoint& outpoint, Coin& coin) const override;
899+    void AddPackageTransaction(const CTransactionRef& tx);
900+    bool PackageContains(uint256 txid) const {
901+        return package_txids.count(txid) != 0;
902+    }

jnewbery commented at 2:32 pm on February 1, 2021:

It looks like this (and by extension, package_txids) is unused.

in src/validation.cpp:656 in 2fac013613 outdated

652@@ -641,6 +653,14 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
653 
654     LockPoints lp;
655     m_view.SetBackend(m_viewmempool);
656+    m_viewmempool.SetBackend(::ChainstateActive().CoinsTip());

jnewbery commented at 2:41 pm on February 1, 2021:

It seems a bit odd that the first time PreChecks() is called, we’re calling SetBackend() when the backend is already set in the constructor for MemPoolAccept, and that we repeatedly call m_view.SetBackend(m_viewmempool)

I suggest that in MemPoolAccept::MemPoolAccept():

set m_viewmempool to be backed by m_dummy
set m_view to be backed by m_viewmempool

and then here, remove the m_view.SetBackend(m_viewmempool) call.

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

Agreed 🧠 will update

in src/validation.cpp:698 in 2fac013613 outdated

698     // block; we don't want our mempool filled up with transactions that can't
699-    // be mined yet.
700-    // Must keep pool.cs for this unless we change CheckSequenceLocks to take a
701-    // CoinsViewCache instead of create its own
702-    if (!CheckSequenceLocks(m_pool, tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp))
703+    // be mined yet. Must keep pool.cs because this uses a CCoinsViewMemPool.

jnewbery commented at 2:57 pm on February 1, 2021:

I think this whole comment about “must keep pool.cs” is obsolete, since pool.cs is kept throughout the MemPoolAccept run.

in src/validation.cpp:1172 in 2fac013613 outdated

1105+        Workspace& workspace = workspaces[i];
1106+
1107+        if (!PolicyScriptChecks(args, workspace, txdata)) {
1108+            return std::vector<MempoolAcceptResult> { MempoolAcceptResult(workspace.m_state, workspace.m_ptx) };
1109+        }
1110+    }

jnewbery commented at 4:01 pm on February 1, 2021:

Consider using range based loops here:

 0    for (Workspace& ws : workspaces) {
 1        if (!PreChecks(args, ws)) {
 2            return std::vector<MempoolAcceptResult> { MempoolAcceptResult(ws.m_state, ws.m_ptx) };
 3        }
 4        m_viewmempool.AddPackageTransaction(ws.m_ptx);
 5    }
 6
 7    // TODO: Enforce package-level feerate and other policies before script checks.
 8    for (Workspace& ws : workspaces) {
 9        PrecomputedTransactionData txdata;
10
11        if (!PolicyScriptChecks(args, ws, txdata)) {
12            return std::vector<MempoolAcceptResult> { MempoolAcceptResult(ws.m_state, ws.m_ptx) };
13        }
14    }

in src/validation.cpp:1315 in 2fac013613 outdated

1151+    assert(test_accept); // Only allow package accept dry-runs (testmempoolaccept RPC).
1152+
1153     const CChainParams& chainparams = Params();
1154-    return AcceptToMemoryPoolWithTime(chainparams, pool, state, tx, GetTime(), plTxnReplaced, bypass_limits, test_accept, fee_out);
1155+    std::vector<COutPoint> coins_to_uncache;
1156+    MemPoolAccept::ATMPArgs args { chainparams, GetTime(), false, coins_to_uncache, test_accept };

jnewbery commented at 4:05 pm on February 1, 2021:

No need for the local temporary chainparams:

0    MemPoolAccept::ATMPArgs args { Params(), GetTime(), false, coins_to_uncache, test_accept };

Same goes for chainparams var in AcceptToMemoryPool() if you want to change it.

in src/rpc/rawtransaction.cpp:988 in 2fac013613 outdated

1026+        }
1027+    } else {
1028+        Assume(validation_results[0].m_failed_ptx);
1029+        UniValue result_0(UniValue::VOBJ);
1030+        result_0.pushKV("txid", validation_results[0].m_failed_ptx->GetHash().GetHex());
1031+        result_0.pushKV("wtxid", validation_results[0].m_failed_ptx->GetWitnessHash().GetHex());

jnewbery commented at 4:18 pm on February 1, 2021:

I’m not sure that I like the interface of passing back a CTransactionRef just for this failure case. I wonder if we should:

allow m_accepted to be tri-state {accepted, failed, not_fully_validated}. That can be achieved with a std::optional<bool>
return a MempoolAcceptResult for all transactions in the package where either:
- all are success
- one is fail and the rest are not_fully_validated
work out the txid of the failing transaction from its index in the vector.

What do you think?

jnewbery commented at 4:19 pm on February 1, 2021: member

I’ve reviewed all the commits apart from the final functional test for packages in RPCs. This is looking really great!

This is turning into quite a large PR to review, with refactors and new functionality. I think it could be split to focus and aid review:

clean up redundant conditional, return MempoolAcceptResult and const ATMPArgs and non-const Workspace could be one PR. They’re fairly mechanical refactors.
remove mviewmempool backend for coins_to_uncache and add CheckSequenceLocks with coinsview could be a second PR. They’re refactors, but they need quite careful consideration because of the way you’re moving around the coins views.
The rest of the commits could be a third PR.

A few other comments:

In commit [refactor] return MempoolAcceptResult, the commit log says “It returns a const result”. I’m not sure that makes sense.
In commit [validation] remove mviewmempool backend for coins_to_uncache, change mviewmempool to m_viewmempool

glozow force-pushed on Feb 2, 2021

glozow marked this as a draft on Feb 2, 2021

glozow commented at 2:07 am on February 2, 2021: member

To all the lovely reviewers, as @jnewbery pointed out, this PR is getting a little big 😅 so I’ve split off the refactoring commits to #21062.

Thanks a ton for the review @fjahr 🙏 I think I’ve addressed all your comments/questions. @jnewbery I’ve addressed most of your comments - I agree with you on returning a vector (all success) || (maybe some succeeded, one failed, and all others not-fully-validated) from ProcessNewPackage. Do you also recommend doing that for the testmempoolaccept API? Marking this as a draft so I can figure this out while the refactors are cooking, heh.

MarcoFalke referenced this in commit 8e1913ae02 on Feb 11, 2021

glozow force-pushed on Feb 11, 2021

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

To reviewers: #21062 is merged. ~Next chunk is #21146, which is centered around MemPoolAccept’s interaction with the coins cache. This PR is up-to-date so feel free to review, but we need #21146 first 😊~

glozow force-pushed on Feb 22, 2021

glozow force-pushed on Feb 23, 2021

glozow force-pushed on Mar 2, 2021

glozow marked this as ready for review on Mar 2, 2021

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

This is ready for review 💃 no blockers anymore The approach has slightly changed - I’m now extending CCoinsViewCache to create a CCoinsViewTemporary (used only in mempool validation) to track package coins. They’re stored at the top layer of the MemPoolAccept coins hierarchy, in m_view instead of m_viewmempool. And we can clear the temporary coins to reset the state without deletingcacheCoins :)

in src/test/miner_tests.cpp:31 in 4b891df741 outdated

27@@ -28,7 +28,8 @@ struct MinerTestingSetup : public TestingSetup {
28     void TestPackageSelection(const CChainParams& chainparams, const CScript& scriptPubKey, const std::vector<CTransactionRef>& txFirst) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs);
29     bool TestSequenceLocks(const CTransaction& tx, int flags) EXCLUSIVE_LOCKS_REQUIRED(::cs_main, m_node.mempool->cs)
30     {
31-        return CheckSequenceLocks(::ChainstateActive(), *m_node.mempool, tx, flags);
32+        CCoinsViewMemPool viewMemPool(&::ChainstateActive().CoinsTip(), *m_node.mempool);

Xekyo commented at 6:43 pm on March 2, 2021:

Nit: I would write mempool as a single word at this point in the lingo evolution. :)

in src/validation.cpp:466 in 6bf3ff6b05 outdated

461@@ -462,6 +462,113 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS
462     return CheckInputScripts(tx, state, view, flags, /* cacheSigStore = */ true, /* cacheFullSciptStore = */ true, txdata);
463 }
464 
465+static const Coin coin_empty;

Xekyo commented at 6:55 pm on March 2, 2021:

Optionally add an introductory comment for coin_empty. Perhaps spent_coin is more speaking.

glozow force-pushed on Mar 2, 2021

glozow commented at 2:59 am on March 3, 2021: member

Fixed CI failure

in src/validation.cpp:485 in 6bf3ff6b05 outdated

495+    // CCoinsViewTemporary on top of a base cache.
496+    CCoinsViewTemporary(const CCoinsViewTemporary &) = delete;
497+
498+    bool GetCoin(const COutPoint& outpoint, Coin& coin) const override {
499+        coin = AccessCoin(outpoint);
500+        return !coin.IsSpent();

Xekyo commented at 8:44 pm on March 3, 2021:

Slightly surprised that the function’s name is get… but it returns a boolean on whether the retrieval was successful. I’d kinda expect a get… function to return the object.

glozow commented at 5:58 pm on March 8, 2021:

Marking as resolved because this wouldn’t be a change for this PR, but yes I agree the CoinsView API is not the most aptly named :)

sipa commented at 6:31 pm on March 8, 2021:

FWIW, there is a historical reason why there are lots of bool Get*(....,&return_object) functions in the codebase. Before C++11’s move semantics it was hard to avoid copies in many cases (needing swaps all over the place…). copy elision existed in C++98, but has been expanded greatly since (and is now mandatory in C++17). It’s also only since C++17 that std::optional exists so there is a clean way of returning an optional value; before that you’d sometimes see an idiom of functions returning a std::pair<value, bool> (e.g. std::set::insert), but without structured binding (also C++17) or even std::tie (C++11) that’s pretty annoying to use too.

TL;DR: there used to not really be a great way of having getters that are both efficient and can fail. With the current language, I think it’d just return std::optional<Coin>.

in src/validation.cpp:506 in 6bf3ff6b05 outdated

500+        return !coin.IsSpent();
501+    }
502+
503+    const Coin& AccessCoin(const COutPoint& outpoint) const override {
504+        // Check to see if the inputs are spent by a tx being considered for a package. When a
505+        // package transaction spends coins from the mempool, it is only reflected in m_temp_spent.

Xekyo commented at 9:01 pm on March 3, 2021:

If this is is supposed to mean that there are two conflicting transactions in the package, this could be phrased a bit more clearly. How about:

“Check if another transaction in the package has already spent the given UTXO. UTXO consumed by this package are only tracked in m_temp_spent.”

in src/validation.cpp:496 in 6bf3ff6b05 outdated

506+        if (m_temp_spent.count(outpoint)) {
507+            return coin_empty;
508+        }
509+
510+        // Check to see if the inputs are in a tx being considered for a package.
511+        // These Coins would not be available in the underlying CoinsView.

Xekyo commented at 9:04 pm on March 3, 2021:

Isn’t only a single package under review here? How about:

“Check whether the input was created by another transaction in the package under review.”

in src/validation.cpp:501 in 6bf3ff6b05 outdated

511+        // These Coins would not be available in the underlying CoinsView.
512+        if (auto it = m_temp_added.find(outpoint); it != m_temp_added.end()) {
513+            assert(!it->second.IsSpent());
514+            return it->second;
515+        }
516+        return CCoinsViewCache::AccessCoin(outpoint);

Xekyo commented at 9:06 pm on March 3, 2021:

It would make more sense to me if the names of GetCoin and AccessCoin were swapped… :confounded:

glozow commented at 5:58 pm on March 8, 2021:

Marking as resolved because this wouldn’t be a change for this PR, but yes I agree the CoinsView API is not the most aptly named :)

DrahtBot added the label Needs rebase on Mar 4, 2021

glozow force-pushed on Mar 4, 2021

DrahtBot removed the label Needs rebase on Mar 4, 2021

in src/validation.cpp:611 in 390574584a outdated

602@@ -603,6 +603,9 @@ class MemPoolAccept
603     // Single transaction acceptance
604     MempoolAcceptResult AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
605 
606+    // Multiple transaction acceptance
607+    std::vector<MempoolAcceptResult> AcceptMultipleTransactions(std::vector<CTransactionRef>& txns, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main);

Xekyo commented at 10:47 pm on March 4, 2021:

It would have helped me if this comment had stated what assumptions the transaction package underlies if any, (e.g. whether they are all part of a connected set or not). I might have expected such assumptions to be explicitly stated in the parameter descriptions of a function in another codebase.

in src/validation.cpp:1256 in 390574584a outdated

1253+                           return MempoolAcceptResult(*ws.m_ptx, std::move(ws.m_replaced_transactions), ws.m_base_fees);
1254+            });
1255+            std::transform(it_curr, workspaces.end(), std::back_inserter(results), failed_or_unfinished);
1256+            return results;
1257+        }
1258+        // Add the coins back because subsequent transaction(s) in the package may need them for

Xekyo commented at 10:56 pm on March 4, 2021:

This sounds like you’re assuming that you’re getting the transactions in topological order. If that’s a requirement, it would be good to list that above as mentioned.

Also, you may want to make your API input checks as lenient as possible and a specific order does not seem necessary to me.

in src/rpc/rawtransaction.cpp:908 in a3264ac359 outdated

891@@ -890,13 +892,15 @@ static RPCHelpMan testmempoolaccept()
892                 },
893                 RPCResult{
894                     RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n"
895-                        "Length is exactly one for now.",
896+                        "Returns results for each transaction in the same order they were passed in.\n"
897+                        "It is possible for transactions to not be fully validated ('allowed' unset) if an earlier transaction failed.\n",

Xekyo commented at 11:04 pm on March 4, 2021:

I thought this RPC was supposed to generally fail if any transactions fail.

glozow commented at 8:14 pm on March 6, 2021:

I changed my mind on it because it seemed like the 1-if-failure API was confusing and less helpful

in test/functional/rpc_packages.py:154 in 61d54b613d outdated

120+            {"txid": tx_high_fee.rehash(), "wtxid": tx_high_fee.getwtxid(), "allowed": False, "reject-reason": "max-fee-exceeded"}
121+        ])
122+        testres_package_high_fee = node.testmempoolaccept(self.independent_txns_hex + [tx_high_fee_signed["hex"]])
123+        assert_equal(testres_package_high_fee, self.independent_txns_testres + testres_high_fee)
124+
125+    def test_chain(self):

Xekyo commented at 11:25 pm on March 4, 2021:

Optional additional tests: • parent with two distinct child transactions • child with two parent transactions

glozow commented at 9:03 pm on March 8, 2021:

• parent with two distinct child transactions • child with two parent transactions

Added both tests. Thanks for the suggestion!

in test/functional/rpc_packages.py:105 in 61d54b613d outdated

77+        tx = CTransaction()
78+        assert signedtx["complete"]
79+        tx.deserialize(BytesIO(hex_str_to_bytes(signedtx["hex"])))
80+        return (tx, signedtx["hex"], tx.vout[0].scriptPubKey.hex())
81+
82+    def test_independent(self):

Xekyo commented at 11:28 pm on March 4, 2021:

Maybe I’m just overlooking it, but what about a valid set of independent transactions? That should be accepted now, but later fail when you require packages to consist of dependent transactions. Especially, I would be interested in seeing that the connectedness test properly recognizes two independent parent-child pairs as unconnected.

glozow commented at 9:09 pm on March 8, 2021:

I’ve been thinking about your comment about making the API more lenient, and I agree that I should always let testmempoolaccept accept lists of transactions that aren’t necessarily connected. My plan for now is to return a depends list for each transaction, which will allow us to test the package-ness of the transactions passed in.

glozow force-pushed on Mar 8, 2021

glozow commented at 9:10 pm on March 8, 2021: member

Addressed @Xekyo’s comments, added more tests

JeremyRubin added this to the "Package Relay" column in a project

in src/validation.cpp:1315 in 06f8fce092 outdated

1310+    AssertLockHeld(cs_main);
1311+    assert(test_accept); // Only allow package accept dry-runs (testmempoolaccept RPC).
1312+
1313+    std::vector<COutPoint> coins_to_uncache;
1314+    const CChainParams& chainparams = Params();
1315+    MemPoolAccept::ATMPArgs args { chainparams, GetTime(), false, coins_to_uncache, test_accept };

ariard commented at 3:15 pm on March 15, 2021:

nit : “/* bypass_limits */ false”

in src/validation.cpp:1218 in 06f8fce092 outdated

1303@@ -1120,6 +1304,30 @@ MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPoo
1304     return AcceptToMemoryPoolWithTime(Params(), pool, active_chainstate, tx, GetTime(), bypass_limits, test_accept);
1305 }
1306 
1307+std::vector<MempoolAcceptResult> ProcessNewPackage(CChainState& active_chainstate, CTxMemPool& pool,
1308+                                                   std::vector<CTransactionRef>& txns, bool test_accept)
1309+{
1310+    AssertLockHeld(cs_main);
1311+    assert(test_accept); // Only allow package accept dry-runs (testmempoolaccept RPC).

ariard commented at 3:39 pm on March 15, 2021:

Should we limit the submitted package size for a reasonable bound for now (MAX_PACKAGE_SIZE) ?

Even if package can only be submitted by the RPC interface, I’m not sure I would qualify this one of fully-trusted. A service provider or L2 node might indirectly call testmempoolaccept on its local node with some package content contributed by a user/counterparty. E.g in LN, the number of HTLCs txn on a given commitment is partially chosen by the counterparty, even if upper bounded by protocol parameter.

I would like to avoid some naive Bitcoin infrastructure opening the DoS surface of their local node by allowing thousands-width package evaluation. We don’t have a visibility on package composition and the simple schemes we have in mind while designing this stuff might not be the one submitted in deployment.

EDIT: I know we have (in theory) the -limitdescendantcount bound , but we might still have DoS around a package-specific data structure, which might be reached without triggering descendants upper bound.

glozow commented at 3:25 pm on March 19, 2021:

Added a max size and max count in the latest push :)

in src/validation.cpp:1144 in 06f8fce092 outdated

1219+    results.reserve(package_size);
1220+
1221+    LOCK(m_pool.cs);
1222+    // Do all PreChecks first and fail fast to avoid running expensive script checks when unnecessary.
1223+    for (Workspace& ws : workspaces) {
1224+        if (!PreChecks(args, ws)) {

ariard commented at 4:14 pm on March 15, 2021:

I think we have an issue with how we’re currently evaluating package in case of conflict.

Let’s say you have a conflicting chain of transaction with your mempool. Conflicting parent transaction has a 5sat/vbyte feerate 300 vbyte size, child transaction has a 20sat/vbyte feerate 400 vbyte size. Absolute fee of this chain of transaction is thus 5 * 300 + 20 * 400 = 9500 sat.

A package is tested for mempool-acceptance with the following composition. Parent tx has a 100sat/vbyte feerate 300 vbyte size, child transaction has a 10sat/vbyte 400 vbyte size. Absolute fee of the parent transaction is thus 30000 sat.

Package parent feerate and absolute fee is superior to the replaced chain of transaction (9500sat). Following bip125 requirement it should be accepted in the mempool, and evict the conflicting chain of transactions. Package child submitted later on, even if its feerate was under the previously-conflicting child (10sat/vbyte vs 20sat/vbyte) is accepted anyway as the parent clears up the way. Note, conflicting transactions are evicted in Finalize().

However, this PR is evaluating for conflict the whole package in the raw, without removing conflicting transaction from previously and successfully evaluated package member. In the above scenario described, I think the package child would fail to be accepted in the mempool, the conflicting child still being present.

Thus, submitting your package via testmempoolaccept or your transaction atomically might yell two differing results, all other things being equal ?

JeremyRubin commented at 1:20 am on April 15, 2021:

I think this is intended and was out of scope of this PR – this is just setup for later expanding what can be accepted

ariard commented at 6:31 pm on April 22, 2021:

Lol, I don’t think it was documented in OP at PR opening ;)

in src/validation.cpp:1234 in 06f8fce092 outdated

1235+    }
1236+
1237+    // Now that we have verified all inputs are available and there are no conflicts in the package,
1238+    // clear the temporary coins (m_temp_added and m_temp_spent), otherwise script checks will error
1239+    // on coins that are spent within the package.
1240+    m_view.ClearTemporaryCoins();

ariard commented at 4:27 pm on March 15, 2021:

I don’t follow the rational to clear the temporary coins “otherwise scripts checks will error on coins that are spent within the package”. If a coin is already spent by a previous input from the same transaction or even another package transaction it’s the expected script checks behavior to fail on it ?

At the contrary, I would understand removing coins to avoid a package parent spending a coin lately added by a package child. Thus I expect that kind of pathological case catched up earlier, when we try to access parent spent coins in PreChecks() ?

glozow commented at 8:31 pm on March 15, 2021:

I’ll update the comment to make it clearer. A lot of functions in the script checks do a quick assert(!coin.IsSpent()) sanity check. This breaks when the coin is spent by a later transaction in the package (and we’ve updated it while going through all the PreChecks)

in src/validation.cpp:1168 in 06f8fce092 outdated

1250+                                              : MempoolAcceptResult(*ws.m_ptx, ws.m_state, /* finished */ false);
1251+            };
1252+            auto it_curr = std::find_if(workspaces.begin(), workspaces.end(),
1253+                                        [& failed_ptx](Workspace& ws) { return ws.m_ptx == failed_ptx; });
1254+            // When test_accept=true, transactions that pass PolicyScriptChecks are valid because there are
1255+            // no further mempool checks (passing PolicyScriptChecks implies passing ConsensusScriptChecks).

ariard commented at 4:42 pm on March 15, 2021:

I think you’re forgetting the mempool size check in Finalize which might exclude your newly package if it was in the feerate-bottom of the mempool. See CTxMemPool::TrimToSize.

Beyond the fact that policy flags are a superset of the consensus ones, you could also recall that’s running ConsensusScriptChecks isn’t worthy here as we don’t try to cache script results. testmempoolaccept transactions are likely never to be broadcasted on the network, so better to spare the cache, IMHO. But maybe precise we should do it when we use this code path for package evaluation from the network ?

glozow commented at 3:20 pm on March 19, 2021:

I think you’re forgetting the mempool size check in Finalize

We have a feerate check against the mempool minimum fee in CheckFeeRate() (in PreChecks()). The trim to size is done after submitting to mempool just in case it wasn’t accurate (and I suspect the mempool lock was perhaps not held the whole time in the past, so it was possible for the mempool to grow in the time we were doing script checks?). On master, a test_accept for one transaction stops short of calling Finalize() and doesn’t do this check either.

I prefer using CheckFeeRate() for a test_accept; there’s no need to submit to mempool, update everything, and then remove it again and re-update everything, just to check size limits. Since we would release the lock at the end of the testmempoolaccept call, it’s possible for the mempool to grow before the user submits it anyway, so it’s not like we’re guaranteed an accurate result by doing this.

glozow commented at 3:25 pm on March 19, 2021:

Beyond the fact that policy flags are a superset of the consensus ones, you could also recall that’s running ConsensusScriptChecks isn’t worthy here as we don’t try to cache script results.

Yep, I agree! See also this discussion.

ariard commented at 6:30 pm on April 22, 2021:

(and I suspect the mempool lock was perhaps not held the whole time in the past, so it was possible for the mempool to grow in the time we were doing script checks?).

Mempool lock was maybe not hold but you would have cs_main as a backup no ? I wouldn’t be surprise of such behavior, but it’s not a behavior on current master (4b5659c) ?

On master, a test_accept for one transaction stops short of calling Finalize() and doesn’t do this check either.

Ah right, so I agree with you package-testmempoolaccept should stick to the same behavior. As a rational for not doing it I can definitely see your explanation as it’s avoid expensive traversals of mempool structure, though at the price of RPC call not exactly matching a “real” submission on an edge-case. It would have been nice to have this documented more clearly by the original `testmempoolaccept PR.

Since we would release the lock at the end of the testmempoolaccept call, it’s possible for the mempool to grow before the user submits it anyway, so it’s not like we’re guaranteed an accurate result by doing this.

Right, but I think we assume that if you submit transaction A through testmempoolaccept then sendrawtransaction acceptance results should match, because all things being equal (no expiry, no new transaction, no node version upgrade, …) ?

glozow commented at 2:44 am on May 6, 2021:

I’ve noted in the release notes that the true mempool minimum fee may not be met in the test_accept

Perhaps in a separate PR (which would be relevant even for non-package-accept) we could figure out how to get a more accurate mempool minimum fee without submitting to it :)

in src/rpc/rawtransaction.cpp:882 in 06f8fce092 outdated

875@@ -876,7 +876,9 @@ static RPCHelpMan sendrawtransaction()
876 static RPCHelpMan testmempoolaccept()
877 {
878     return RPCHelpMan{"testmempoolaccept",
879-                "\nReturns result of mempool acceptance tests indicating if raw transaction (serialized, hex-encoded) would be accepted by mempool.\n"
880+                "\nReturns result of mempool acceptance tests indicating if raw transaction(s) (serialized, hex-encoded) would be accepted by mempool.\n"
881+                "\nIf multiple transactions are passed in, they must be sorted in order of dependency and not conflict with each other.\n"
882+                "\nThe maximum number of transactions allowed is determined by the mempool descendant policy (-limitdescendantcount).\n"

ariard commented at 4:56 pm on March 15, 2021:

Do you have test coverage for this ?

CalculateMemPoolAncestors is running on m_pool which is updated with validated transaction only in Finalize. I don’t see how a package transaction is successfully accounted in descendant/ancestor limits in the subsequent evaluation of the package…

glozow commented at 8:36 pm on March 18, 2021:

Do you have test coverage for this ?

Yes, I update the test case in mempool_accept.py, it should return a JSON Serialization Error for > 25 transactions.

Will be pushing an update to add package count/size limits in the validation code.

in src/validation.cpp:467 in 06f8fce092 outdated

462@@ -465,6 +463,114 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS
463     return CheckInputScripts(tx, state, view, flags, /* cacheSigStore = */ true, /* cacheFullSciptStore = */ true, txdata);
464 }
465 
466+/** An empty coin used as a placeholder for a spent coin.*/
467+static const Coin coin_spent;

ariard commented at 5:05 pm on March 15, 2021:

I think this is only needed for spentness evaluation in GetCoin ? If so just make it part of the new class.

jnewbery commented at 11:09 am on March 17, 2021:

Indeed:

 0-/** An empty coin used as a placeholder for a spent coin.*/
 1-static const Coin coin_spent;
 2 /**
 3  * A CoinsView that adds a memory cache to another CoinsView and serves as temporary scratch space.
 4  * Used by MemPoolAccept class to validate transactions and packages before submitting to mempool.
 5@@ -489,6 +487,8 @@ protected:
 6     */
 7     std::set<COutPoint> m_temp_spent;
 8 
 9+    /** An empty coin used as a placeholder for a spent coin.*/
10+    inline static const Coin s_coin_spent;
11 public:
12 
13     CCoinsViewTemporary(CCoinsView* baseIn) : CCoinsViewCache(baseIn) {}
14@@ -506,7 +506,7 @@ public:
15         // Check to see if another tx in the package has already spent this coin (conflict-in-package).
16         // Coins spent by others in the package are only tracked in m_temp_spent.
17         if (m_temp_spent.count(outpoint)) {
18-            return coin_spent;
19+            return s_coin_spent;
20         }

in src/validation.cpp:477 in 06f8fce092 outdated

472+ * the backend is disabled. Avoid using a CCoinsViewTemporary in consensus-critical paths such
473+ * as writing to the script cache. See CheckInputsFromMempoolAndCache as an example. When not being
474+ * used to validate a package (m_temp_added and m_temp_spent are empty), a CCoinsViewTemporary
475+ * behaves exactly like a CCoinsViewCache.
476+ */
477+class CCoinsViewTemporary : public CCoinsViewCache

ariard commented at 5:06 pm on March 15, 2021:

Code-organisation wise, maybe better to move it in coins.h, validation.cpp is already wide enough ?

glozow commented at 8:29 pm on March 15, 2021:

I actually think it’s better organization to put it in validation.cpp, since it’s only used by MemPoolAccept.

jnewbery commented at 11:17 am on March 17, 2021:

We’ve stopped using hungarian style naming, so this should probably just be called CoinsViewTemporary (the C indicated “class”)

in src/validation.h:240 in 06f8fce092 outdated

229@@ -226,6 +230,18 @@ struct MempoolAcceptResult {
230 MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
231                                        bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
232 
233+/**
234+* Atomically test acceptance of multiple transactions.
235+* @param[in]    txns                Group of transactions which may be independent or contain

ariard commented at 5:12 pm on March 15, 2021:

I think we should think twice w.r.t independence as the evaluation of topologically-independent packages might still have interdependent evaluations.

Let’s say your package 1 is replacing in-mempool tx A. Such high-feerate tx A is a replacement blocker for the low-feerate package 2. Evaluating package 1 then package 2 make both of them accepted, the reserve isn’t true.

Also it make it harder to reason on package upper bound size, as you might have N independent packages all under descendant limit but collectively far above.

glozow commented at 1:03 pm on April 13, 2021:

Current policy is they can be independent or dependent (although I don’t think the user would get much use out of independent ones). Given that RBF is now disabled, hopefully this concern is resolved?

in src/validation.h:279 in 06f8fce092 outdated

275@@ -258,11 +276,11 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE
276  * See consensus/consensus.h for flag definitions.
277  */
278 bool CheckSequenceLocks(CChainState& active_chainstate,
279-                        const CTxMemPool& pool,
280+                        CCoinsView& viewMemPool,

jnewbery commented at 10:50 am on March 17, 2021:

Can this be a const reference?

0                        const CCoinsView& viewMemPool,

Passing by const reference indicates that the argument won’t be mutated in the function, and is the preferred way of passing in-params that are not cheap to copy (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-in)

in src/validation.cpp:488 in 06f8fce092 outdated

485+
486+    /**
487+    * Coins spent by transactions being validated. When there are multiple, we need to track these
488+    * in order to distinguish between missing/spent coins and conflicts within a package.
489+    */
490+    std::set<COutPoint> m_temp_spent;

jnewbery commented at 11:14 am on March 17, 2021:

You could combine these into a single std::set<COutPoint, std::optional<Coin>> (where nullopt indicates that the coin has been spent). That’d avoid doing lookups in both places. Up to you whether you think that’s clearer or not.

EDIT: Or even just use std::set<COutPoint, Coin>, where the Coin is an empty (spent) coin if the coin has been spent.

glozow commented at 11:58 pm on March 29, 2021:

Thanks for the suggestion! I considered this when I first saw the comment, but since then, I’ve updated AcceptMultipleTransactions() to just remove m_temp_spent after all the PreChecks() so that the script checks can use coins from m_temp_added. That makes this suggestion no longer apply so I’m going to mark it as resolved.

in src/validation.cpp:487 in 06f8fce092 outdated

482+    * validation, since we can access transaction outputs without submitting them to mempool.
483+    */
484+    std::map<COutPoint, Coin> m_temp_added;
485+
486+    /**
487+    * Coins spent by transactions being validated. When there are multiple, we need to track these

jnewbery commented at 11:16 am on March 17, 2021:

0    * Coins spent by transactions being validated. When validating a package, we need to track these

in src/validation.cpp:479 in 06f8fce092 outdated

474+ * used to validate a package (m_temp_added and m_temp_spent are empty), a CCoinsViewTemporary
475+ * behaves exactly like a CCoinsViewCache.
476+ */
477+class CCoinsViewTemporary : public CCoinsViewCache
478+{
479+protected:

jnewbery commented at 11:28 am on March 17, 2021:

Doesn’t need to be protected. Nothing inherits from this class so it can use (default) private specifier.

in src/validation.h:202 in 06f8fce092 outdated

201         INVALID, //!> Invalid.
202+        UNFINISHED, //!> Not fully validated.
203     };
204-    ResultType m_result_type;
205-    TxValidationState m_state;
206+    const CTransaction& m_tx;

jnewbery commented at 11:53 am on March 17, 2021:

It makes me slightly uneasy that we’re returning a reference to a CTransaction object which was passed in to ProcessNewPackage(). The caller needs to know that they can’t let that passed-in CTransaction go out of scope, or they’ll have a dangling reference.

What do you think about passing back the txid, and then having the caller look up the txid/wtxid/virtual size from the CTransaction object that they’re holding? Or passing back those details in the MempoolAcceptResult?

in src/validation.cpp:1179 in 06f8fce092 outdated

1175@@ -1060,28 +1176,93 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
1176 
1177     Workspace ws(ptx);
1178 
1179-    if (!PreChecks(args, ws)) return MempoolAcceptResult(ws.m_state);
1180+    if (!PreChecks(args, ws)) return MempoolAcceptResult(*ws.m_ptx, ws.m_state);

jnewbery commented at 12:11 pm on March 17, 2021:

It’s not immediately obvious what the different constructor calls here indicate. Perhaps adding factory methods to the struct would make it a bit clearer:

  0diff --git a/src/validation.cpp b/src/validation.cpp
  1index 121181393d..c65964a976 100644
  2--- a/src/validation.cpp
  3+++ b/src/validation.cpp
  4@@ -1176,7 +1176,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
  5 
  6     Workspace ws(ptx);
  7 
  8-    if (!PreChecks(args, ws)) return MempoolAcceptResult(*ws.m_ptx, ws.m_state);
  9+    if (!PreChecks(args, ws)) return MempoolAcceptResult::Failure(*ws.m_ptx, ws.m_state);
 10 
 11     // Only compute the precomputed transaction data if we need to verify
 12     // scripts (ie, other policy checks pass). We perform the inexpensive
 13@@ -1184,20 +1184,20 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
 14     // checks pass, to mitigate CPU exhaustion denial-of-service attacks.
 15     PrecomputedTransactionData txdata;
 16 
 17-    if (!PolicyScriptChecks(args, ws, txdata)) return MempoolAcceptResult(*ws.m_ptx, ws.m_state);
 18+    if (!PolicyScriptChecks(args, ws, txdata)) return MempoolAcceptResult::Failure(*ws.m_ptx, ws.m_state);
 19 
 20-    if (!ConsensusScriptChecks(args, ws, txdata)) return MempoolAcceptResult(*ws.m_ptx, ws.m_state);
 21+    if (!ConsensusScriptChecks(args, ws, txdata)) return MempoolAcceptResult::Failure(*ws.m_ptx, ws.m_state);
 22 
 23     // Tx was accepted, but not added
 24     if (args.m_test_accept) {
 25-        return MempoolAcceptResult(*ws.m_ptx, std::move(ws.m_replaced_transactions), ws.m_base_fees);
 26+        return MempoolAcceptResult::Success(*ws.m_ptx, std::move(ws.m_replaced_transactions), ws.m_base_fees);
 27     }
 28 
 29-    if (!Finalize(args, ws)) return MempoolAcceptResult(*ws.m_ptx, ws.m_state);
 30+    if (!Finalize(args, ws)) return MempoolAcceptResult::Failure(*ws.m_ptx, ws.m_state);
 31 
 32     GetMainSignals().TransactionAddedToMempool(ptx, m_pool.GetAndIncrementSequence());
 33 
 34-    return MempoolAcceptResult(*ws.m_ptx, std::move(ws.m_replaced_transactions), ws.m_base_fees);
 35+    return MempoolAcceptResult::Success(*ws.m_ptx, std::move(ws.m_replaced_transactions), ws.m_base_fees);
 36 }
 37 
 38 std::vector<MempoolAcceptResult> MemPoolAccept::AcceptMultipleTransactions(std::vector<CTransactionRef>& txns, ATMPArgs& args)
 39@@ -1219,8 +1219,8 @@ std::vector<MempoolAcceptResult> MemPoolAccept::AcceptMultipleTransactions(std::
 40             // Exit early to avoid doing pointless work. Return results in the same order as input txns.
 41             const auto failed_or_unfinished = [&, failed_ptx = ws.m_ptx](Workspace& ws) {
 42                                               return ws.m_ptx == failed_ptx
 43-                                              ? MempoolAcceptResult(*ws.m_ptx, ws.m_state, /* finished */ true)
 44-                                              : MempoolAcceptResult(*ws.m_ptx, ws.m_state, /* finished */ false);
 45+                                              ? MempoolAcceptResult::Failure(*ws.m_ptx, ws.m_state, /* finished */ true)
 46+                                              : MempoolAcceptResult::Failure(*ws.m_ptx, ws.m_state, /* finished */ false);
 47             };
 48             std::transform(workspaces.begin(), workspaces.end(), std::back_inserter(results), failed_or_unfinished);
 49             return results;
 50@@ -1240,15 +1240,15 @@ std::vector<MempoolAcceptResult> MemPoolAccept::AcceptMultipleTransactions(std::
 51             CTransactionRef failed_ptx = ws.m_ptx;
 52             const auto failed_or_unfinished = [&failed_ptx](Workspace& ws) {
 53                                               return ws.m_ptx == failed_ptx
 54-                                              ? MempoolAcceptResult(*ws.m_ptx, ws.m_state, /* finished */ true)
 55-                                              : MempoolAcceptResult(*ws.m_ptx, ws.m_state, /* finished */ false);
 56+                                              ? MempoolAcceptResult::Failure(*ws.m_ptx, ws.m_state, /* finished */ true)
 57+                                              : MempoolAcceptResult::Failure(*ws.m_ptx, ws.m_state, /* finished */ false);
 58             };
 59             auto it_curr = std::find_if(workspaces.begin(), workspaces.end(),
 60                                         [& failed_ptx](Workspace& ws) { return ws.m_ptx == failed_ptx; });
 61             // When test_accept=true, transactions that pass PolicyScriptChecks are valid because there are
 62             // no further mempool checks (passing PolicyScriptChecks implies passing ConsensusScriptChecks).
 63             std::transform(workspaces.begin(), it_curr, std::back_inserter(results), [](Workspace& ws) {
 64-                           return MempoolAcceptResult(*ws.m_ptx, std::move(ws.m_replaced_transactions), ws.m_base_fees);
 65+                           return MempoolAcceptResult::Success(*ws.m_ptx, std::move(ws.m_replaced_transactions), ws.m_base_fees);
 66             });
 67             std::transform(it_curr, workspaces.end(), std::back_inserter(results), failed_or_unfinished);
 68             return results;
 69@@ -1260,7 +1260,7 @@ std::vector<MempoolAcceptResult> MemPoolAccept::AcceptMultipleTransactions(std::
 70 
 71     std::transform(workspaces.begin(), workspaces.end(), std::back_inserter(results), [](Workspace& ws) {
 72         // All successful MemPoolAcceptResults
 73-        return MempoolAcceptResult(*ws.m_ptx, std::move(ws.m_replaced_transactions), ws.m_base_fees);
 74+        return MempoolAcceptResult::Success(*ws.m_ptx, std::move(ws.m_replaced_transactions), ws.m_base_fees);
 75     });
 76     return results;
 77 }
 78diff --git a/src/validation.h b/src/validation.h
 79index 64f1b24743..234c1db895 100644
 80--- a/src/validation.h
 81+++ b/src/validation.h
 82@@ -209,8 +209,19 @@ struct MempoolAcceptResult {
 83     /** Raw base fees in satoshis. */
 84     const std::optional<CAmount> m_base_fees;
 85 
 86+    static MempoolAcceptResult Success(const CTransaction& tx, std::list<CTransactionRef>&& replaced_txns, CAmount fees)
 87+    {
 88+        return MempoolAcceptResult(tx, std::move(replaced_txns), fees);
 89+    }
 90+
 91+    static MempoolAcceptResult Failure(const CTransaction& tx, TxValidationState state, bool finished=true)
 92+    {
 93+        return MempoolAcceptResult(tx, state, finished);
 94+    }
 95+
 96+private:
 97     /** Constructor for failure case */
 98-    explicit MempoolAcceptResult(const CTransaction& tx, TxValidationState state, bool finished=true)
 99+    explicit MempoolAcceptResult(const CTransaction& tx, TxValidationState state, bool finished)
100         : m_tx(tx), m_result_type(finished ? ResultType::INVALID : ResultType::UNFINISHED),
101         m_state(state), m_replaced_transactions(nullopt), m_base_fees(nullopt) {
102             if (finished) Assume(!state.IsValid()); // Can be invalid or error

glozow commented at 9:20 pm on March 17, 2021:

Ooh that’s nice! 👀

in src/validation.cpp:1220 in 06f8fce092 outdated

1221+    LOCK(m_pool.cs);
1222+    // Do all PreChecks first and fail fast to avoid running expensive script checks when unnecessary.
1223+    for (Workspace& ws : workspaces) {
1224+        if (!PreChecks(args, ws)) {
1225+            // Exit early to avoid doing pointless work. Return results in the same order as input txns.
1226+            const auto failed_or_unfinished = [&, failed_ptx = ws.m_ptx](Workspace& ws) {

jnewbery commented at 12:18 pm on March 17, 2021:

It’s confusing that you’re using the same variable name ws in the for loop and as an argument to this lambda.

in src/validation.h:213 in 06f8fce092 outdated

217+    const std::optional<CAmount> m_base_fees;
218 
219     /** Constructor for failure case */
220-    explicit MempoolAcceptResult(TxValidationState state)
221-        : m_result_type(ResultType::INVALID),
222+    explicit MempoolAcceptResult(const CTransaction& tx, TxValidationState state, bool finished=true)

jnewbery commented at 12:25 pm on March 17, 2021:

I wonder if we should have a separate constructor for the UNFINISHED state. It doesn’t really make sense to pass in a TxValidationState object if acceptance testing is incomplete.

in src/validation.cpp:1158 in 06f8fce092 outdated

1237+    // Now that we have verified all inputs are available and there are no conflicts in the package,
1238+    // clear the temporary coins (m_temp_added and m_temp_spent), otherwise script checks will error
1239+    // on coins that are spent within the package.
1240+    m_view.ClearTemporaryCoins();
1241+
1242+    for (Workspace& ws : workspaces) {

jnewbery commented at 12:31 pm on March 17, 2021:

I love std::transform more than most, but I think this would be simpler if you just pushed the results onto results as you went (and then filled in the remaining results as UNFINISHED if you fail on any tx).

jnewbery commented at 12:36 pm on March 17, 2021: member

Looking really good. Some comments inline.

DrahtBot added the label Needs rebase on Mar 17, 2021

glozow force-pushed on Mar 19, 2021

DrahtBot removed the label Needs rebase on Mar 19, 2021

in src/packages.h:11 in 917d4b7cc5 outdated

0@@ -0,0 +1,31 @@
1+// Copyright (c) 2021 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_PACKAGES_H
6+#define BITCOIN_PACKAGES_H
7+
8+#include <consensus/validation.h>
9+#include <primitives/transaction.h>

jnewbery commented at 10:19 am on March 19, 2021:

0#include <vector>
1
2#include <consensus/validation.h>
3#include <primitives/transaction.h>

jnewbery commented at 10:18 am on April 23, 2021:

Oops sorry, I’ve led you astray here. Local includes should go before standard library includes :grimacing:

in src/packages.h:12 in 917d4b7cc5 outdated

 7+
 8+#include <consensus/validation.h>
 9+#include <primitives/transaction.h>
10+
11+/** Default maximum number of transactions in a package. */
12+static const unsigned int MAX_PACKAGE_COUNT = 25;

jnewbery commented at 10:20 am on March 19, 2021:

Can be constexpr, uint32_t and brace initialized :grimacing:

0static constexpr uint32_t MAX_PACKAGE_COUNT{25};

in src/rpc/rawtransaction.cpp:965 in 917d4b7cc5 outdated

 998+        if (validation_result.m_tx_results.find(tx->GetHash()) == validation_result.m_tx_results.end()) {
 999+            // Validation unfinished. Just return the txid and wtxid.
1000+            rpc_result.push_back(result_inner);
1001+            continue;
1002+        }
1003+        const auto& accept_result = validation_result.m_tx_results.find(tx->GetHash())->second;

jnewbery commented at 10:24 am on March 19, 2021:

No need to do two lookups into the map:

0        auto it = validation_result.m_tx_results.find(tx->GetHash());
1        if (it == validation_result.m_tx_results.end()) {
2            // Validation unfinished. Just return the txid and wtxid.
3            rpc_result.push_back(result_inner);
4            continue;
5        }
6        const auto& accept_result = it->second;

in src/rpc/rawtransaction.cpp:952 in 917d4b7cc5 outdated

985-            result_0.pushKV("reject-reason", "missing-inputs");
986+        txns.emplace_back(MakeTransactionRef(std::move(mtx)));
987+    }
988+
989+    const PackageMempoolAcceptResult& validation_result =
990+        WITH_LOCK(cs_main, return ProcessNewPackage(::ChainstateActive(), mempool, txns, /* test_accept */ true));

jnewbery commented at 10:35 am on March 19, 2021:

Perhaps avoid using the global ::ChainstateActive() function to avoid conflict with #21391

 0diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
 1index cddb082c2f..1ad0b16545 100644
 2--- a/src/rpc/rawtransaction.cpp
 3+++ b/src/rpc/rawtransaction.cpp
 4@@ -938,6 +938,8 @@ static RPCHelpMan testmempoolaccept()
 5                                              CFeeRate(AmountFromValue(request.params[1]));
 6 
 7     CTxMemPool& mempool = EnsureMemPool(request.context);
 8+    CChainState& chainstate = EnsureChainman(request.context).ActiveChainstate();
 9     std::vector<CTransactionRef> txns;
10 
11     for (unsigned int i = 0; i < raw_transactions.size(); ++i) {
12@@ -949,7 +951,7 @@ static RPCHelpMan testmempoolaccept()
13     }
14 
15     const PackageMempoolAcceptResult& validation_result =
16-        WITH_LOCK(cs_main, return ProcessNewPackage(::ChainstateActive(), mempool, txns, /* test_accept */ true));
17+        WITH_LOCK(cs_main, return ProcessNewPackage(chainstate, mempool, txns, /* test_accept */ true));
18 
19     UniValue rpc_result(UniValue::VARR);

glozow commented at 3:12 pm on March 19, 2021:

gah of course, my bad 🤦 will do

in src/validation.h:250 in 917d4b7cc5 outdated

256+                                        std::map<const uint256, const MempoolAcceptResult>&& results)
257+        : m_state{state}, m_tx_results(std::move(results)) {}
258+
259+    /** Constructor to create a PackageMempoolAcceptResult from a single MempoolAcceptResult */
260+    explicit PackageMempoolAcceptResult(const uint256& txid, const MempoolAcceptResult& result)
261+        : m_state{}, m_tx_results{ {txid, result} } {}

jnewbery commented at 10:45 am on March 19, 2021:

No need to specify default initialization in the initializer list. It’ll happen anyway:

0        : m_tx_results{ {txid, result} } {}

in src/validation.cpp:1332 in 917d4b7cc5 outdated

1327+    // Ensure the cache is still within its size limits.
1328+    for (const COutPoint& hashTx : coins_to_uncache) {
1329+        active_chainstate.CoinsTip().Uncache(hashTx);
1330+    }
1331+    BlockValidationState state_dummy;
1332+    ::ChainstateActive().FlushStateToDisk(chainparams, state_dummy, FlushStateMode::PERIODIC);

jnewbery commented at 10:50 am on March 19, 2021:

Avoid the global ::ChainstateActive() function:

0    active_chainstate.FlushStateToDisk(chainparams, state_dummy, FlushStateMode::PERIODIC);

in src/validation.cpp:1321 in 917d4b7cc5 outdated

1316+
1317+    std::vector<COutPoint> coins_to_uncache;
1318+    const CChainParams& chainparams = Params();
1319+    MemPoolAccept::ATMPArgs args { chainparams, GetTime(), /* bypass_limits */ false, coins_to_uncache, test_accept };
1320+    assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));
1321+    const PackageMempoolAcceptResult& result = package.size() > 1

jnewbery commented at 10:53 am on March 19, 2021:

This looks unsafe to me. You’re taking a reference to a temporary (the return value from AcceptMultipleTransactions() or PackageMempoolAcceptResult())

jnewbery commented at 10:53 am on March 19, 2021:

What happens here if you always call AcceptMultipleTransactions() (even for a single transaction?)

glozow commented at 3:12 pm on March 19, 2021:

The way I have it right now, it AcceptMultipleTransactions() should give an identical result to AcceptSingleTransaction() except that multiple allows 101KvB total size (so theoretically you could have 1 tx of size 101KvB)… can’t think of any other differences. But should the rules diverge further for 1 tx vs package, it’ll cause some of the current testmempoolaccept tests to fail. I was playing around with disabling RBF for packages and ran into that, so I wanted to separate the code paths

jnewbery commented at 5:26 pm on March 19, 2021:

I was wrong. The lifetime of the temporary is extended by binding it to the const lvalue reference: https://en.cppreference.com/w/cpp/language/lifetime#Temporary_object_lifetime.

Still, I think it’d be better to make result a PackageMempoolAcceptResult. I think RVO will mean it doesn’t result in an extra copy.

glozow commented at 11:51 pm on March 29, 2021:

chonged to be a PackageMempoolAcceptResult

in src/validation.cpp:1270 in 917d4b7cc5 outdated

1271+        // no further mempool checks (passing PolicyScriptChecks implies passing ConsensusScriptChecks).
1272+        results.emplace(ws.m_ptx->GetHash(),
1273+                        MempoolAcceptResult::Success(std::move(ws.m_replaced_transactions), ws.m_base_fees));
1274+    }
1275+
1276+    m_view.ClearTemporaryCoins();

jnewbery commented at 11:01 am on March 19, 2021:

Is this needed?

glozow commented at 11:49 pm on March 29, 2021:

(removed for now)

glozow force-pushed on Mar 29, 2021

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

ariard commented at 3:12 am on April 2, 2021: member

Thanks for the update, I’ll review back soon, especially the well-foundness of new packages limits (MAX_PACKAGE_COUNT/MAX_PACKAGE_SIZE).

What do you think about disabling RBF logic for package-testmempoolaccept ? I.e returning a package failure if one utxo is already spent, no matter the signaling of the conflicting transaction. It would fix this current issue and I believe we’ll need to rethink rbf-handling of package in future works anyway.

Also, as L2 protocols are in fine transactions caching layers, if while testing pre-signed transactions testmempool acceptance you already have a conflicting transactions, that’s a hint something is wrong. Like either the off-chain contract is already broken or your private keys for the shared-utxo have already leaked.

Beyond what do you think about auditing how current mempool logic is servicing L2s before to complexify further with this PR ? We might already have issues there.

in src/validation.cpp:535 in 78ab8b1da1 outdated

530+    void PackageAddTransaction(const CTransactionRef& tx)
531+    {
532+        // Track Coins spent by this transaction. They must exist and not already be spent.
533+        for (auto input : tx->vin) {
534+            Coin spent_coin;
535+            Assume(GetCoin(input.prevout, spent_coin) && !spent_coin.IsSpent());

achow101 commented at 4:50 pm on April 2, 2021:

In 78ab8b1da104e76445e65aa7e2db54d338e0b99a “[validation] add CoinsViewTemporary for mempool validation”

&& !spend_coin.IsSpent() seems to be unnecessary as GetCoin returns !coin.IsSpent().

glozow commented at 12:44 pm on April 13, 2021:

Done

in src/validation.cpp:1084 in f7880be3cb outdated

1210+    AssertLockHeld(cs_main);
1211+
1212+    PackageValidationState package_state;
1213+    // Check static package limits before taking the mempool lock.
1214+    const unsigned int package_count = txns.size();
1215+    if (package_count > MAX_PACKAGE_COUNT) {

achow101 commented at 5:46 pm on April 2, 2021:

In f7880be “[validation] package validation test_accept=true”

To more accurately reflect mempool acceptance behavior, I think it would be better to be using the values for -limitancestorcount, -limitdescendantcount, -limitancestorsize, and -limitdescendantsize for these checks.

glozow commented at 10:07 pm on April 2, 2021:

Descendant/ancestor limits will still be checked within validation (and are stricter than this check since this doesn’t include in-mempool transactions). This is just preventing us from working on a really big package. And if/when we have p2p packages, we’d enforce MAX_PACKAGE_COUNT as a limit.

in src/rpc/rawtransaction.cpp:941 in 1bec7b18ab outdated

946                                              CFeeRate(AmountFromValue(request.params[1]));
947 
948     CTxMemPool& mempool = EnsureMemPool(request.context);
949-    int64_t virtual_size = GetVirtualTransactionSize(*tx);
950-    CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size);
951+    CChainState& chainstate = EnsureChainman(request.context).ActiveChainstate();

achow101 commented at 5:50 pm on April 2, 2021:

In 1bec7b18ab8a6e8993fcb6c19a0b13fb4df0eeeb “[rpc] allow multiple txns in testmempoolaccept”

Generally we want to do all of the transaction decoding before getting all of these mempool and chainstate things.

jnewbery commented at 7:45 am on April 3, 2021:

Why? We can’t do anything in this function if there isn’t a mempool, so why not exit early if it doesn’t exist?

achow101 commented at 5:31 pm on April 3, 2021:

The pattern that we do in every other RPC that handles raw transactions (and most of the ones that take user input that needs to be parsed) is that the parsing/decoding is done first before acquiring anything internal needed for processing. After all, we can’t do anything in this function if the transaction is malformed, so why not exit early if it can’t be decoded?

I also think that it would make the diff slightly easier to read since that is what the function did originally.

jnewbery commented at 8:43 am on April 9, 2021:

I see these Assume functions as essentially asserting entry conditions for the function. For rpc methods that require a component to exist in order to function at all (such as testmempoolaccept), then I think assume/exit at the top of the function makes most sense. Doing that has benefits:

avoids unexpected side-effects
avoid unnecessary lock-taking or resource usage
explicitly states assumptions up front

There are some rpc methods where components are optional or only required based on particular inputs (eg getrawtransaction might not require a mempool if a blockhash is passed). For those, the Assume() function can be further down in the function, but again should be prominently displayed at the top of a code block to document assumptions.

glozow commented at 10:47 pm on April 2, 2021: member

What do you think about disabling RBF logic for package-testmempoolaccept ? I.e returning a package failure if one utxo is already spent, no matter the signaling of the conflicting transaction. It would fix this current issue and I believe we’ll need to rethink rbf-handling of package in future works anyway.

Yes, I’d prefer this. I don’t think there’s a use case for having RBFing in packages - if there is, we can add it later after thinking through the edge cases. Will update this to disable RBF.

glozow force-pushed on Apr 13, 2021

glozow commented at 12:57 pm on April 13, 2021: member

Thanks for the reviews so far! Pushed a bunch of stuff (mostly tests):

Separated package limits, sorting, and RBF stuff for packages as [policy] since they are opinionated.
Added the logic to disallow all conflicts with mempool transactions (thereby disabling RBF) in packages.
Extended the mempool fuzzer to check that test accepting using ATMP and ProcessNewPackage (always just 1 transaction) should be identical. Will be sure to update with package submission when I add that (in a future PR).
Added some logic + tests for detecting mempool and package ancestor/descendant limits. I’m using a heuristic where I count every transaction in the package as each other’s ancestor and descendant. The idea is that their total ancestor and descendant chains should never exceed what we’d allow for a single transaction without exhaustively checking every ancestor/descendant count. The heuristic can overestimate the “family” sizes for packages of size > 2 where not every transaction is interdependent, but I don’t think it’s significant for the use cases we care about (usually a package of parent + child).

in src/packages.h:29 in cc00a859de outdated

24+    PCKG_POLICY,                  //!< The package itself is invalid (e.g. too many transactions).
25+    PCKG_TX,                      //!< At least one tx is invalid.
26+};
27+
28+/** A package is an ordered list of transactions. */
29+using Package = std::vector<CTransactionRef>;

JeremyRubin commented at 9:19 pm on April 14, 2021:

suggestion: make Package a struct wrapper with private fields so that you have to go through a smart constructor?

prevents API misuse

glozow commented at 5:14 pm on April 16, 2021:

Are you saying make Package a struct/class? What do you mean by smart constructor, like make_unique/make_shared?

in src/rpc/rawtransaction.cpp:884 in cc00a859de outdated

879@@ -879,7 +880,9 @@ static RPCHelpMan sendrawtransaction()
880 static RPCHelpMan testmempoolaccept()
881 {
882     return RPCHelpMan{"testmempoolaccept",
883-                "\nReturns result of mempool acceptance tests indicating if raw transaction (serialized, hex-encoded) would be accepted by mempool.\n"
884+                "\nReturns result of mempool acceptance tests indicating if raw transaction(s) (serialized, hex-encoded) would be accepted by mempool.\n"
885+                "\nIf multiple transactions are passed in, they must be sorted in order of dependency and not conflict with each other.\n"

JeremyRubin commented at 9:21 pm on April 14, 2021:

nit: clarify which order that is (e.g., if tx B and C are a child of A the order may be [A, B, C] or [A, C, B])

JeremyRubin commented at 9:22 pm on April 14, 2021:

nit: clarify is duplicate transactions [A, A] are allowed

in src/rpc/rawtransaction.cpp:962 in cc00a859de outdated

983-            fees.pushKV("base", ValueFromAmount(fee));
984-            result_0.pushKV("fees", fees);
985+    CTxMemPool& mempool = EnsureMemPool(request.context);
986+    CChainState& chainstate = EnsureChainman(request.context).ActiveChainstate();
987+    const PackageMempoolAcceptResult validation_result = txns.size() > 1
988+        ? WITH_LOCK(cs_main, return ProcessNewPackage(chainstate, mempool, txns, /* test_accept */ true))

JeremyRubin commented at 9:34 pm on April 14, 2021:

Not sure if we do this elsewhere, but would be nice to not take locks inside a ternary with returns in it.

glozow commented at 5:15 pm on April 16, 2021:

It’s the only way i could think of to keep validation_result a const 🤷 could also do a lambda, which is probably less clean:

0const PackageMempoolAcceptResult validation_result = [&]() {
1    if (txns.size() > 1) return WITH_LOCK(::cs_main, return ProcessNewPackage());
2    return WITH_LOCK(::cs_main, return ATMP());
3}();

jnewbery commented at 11:10 am on April 23, 2021:

or:

0const PackageMempoolAcceptResult validation_result = [&]() {
1    LOCK(::cs_main);
2    if (txns.size() > 1) return PNP());
3    return ATMP();
4}();

I think any of them are fine.

in src/rpc/rawtransaction.cpp:956 in cc00a859de outdated

984-            result_0.pushKV("fees", fees);
985+    CTxMemPool& mempool = EnsureMemPool(request.context);
986+    CChainState& chainstate = EnsureChainman(request.context).ActiveChainstate();
987+    const PackageMempoolAcceptResult validation_result = txns.size() > 1
988+        ? WITH_LOCK(cs_main, return ProcessNewPackage(chainstate, mempool, txns, /* test_accept */ true))
989+        : WITH_LOCK(cs_main, return PackageMempoolAcceptResult(txns[0]->GetHash(),

JeremyRubin commented at 9:36 pm on April 14, 2021:

I think we should use ProcessNewPackage here – if this is done to avoid breaking RPC change, then I’d rather a new RPC call that consistently uses one call or the other.

Otherwise how to test if a single tx as a package is accepted?

glozow commented at 10:02 pm on April 14, 2021:

Yeah, there’s a breaking RPC change if they try to testmempoolaccept on a tx that does an RBF (since we don’t allow that in packages). I’m not really sure why a user would want to test a single tx as a package?

JeremyRubin commented at 1:33 am on April 15, 2021:

maybe add a forceaspackage param bool? Would be good to be able to test the single tx as a package code path to test your PR, and a user would want to the same behavior call independent.

Eventually I think all relay should be going through the package path so I like to minimize special casing :)

glozow commented at 9:10 pm on April 15, 2021:

Oh I wasn’t thinking of it that way - why should all relay go through the package path?

For tests, it’s definitely possible to call ProcessNewPackage with a package of 1 transaction, it’s just not exposed through RPC. I have the fuzzer doing it (see top commit)

in src/validation.h:174 in cc00a859de outdated

187@@ -187,34 +188,65 @@ void PruneBlockFilesManual(CChainState& active_chainstate, int nManualPruneHeigh
188 * Validation result for a single transaction mempool acceptance.
189 */
190 struct MempoolAcceptResult {
191-    /** Used to indicate the results of mempool validation,
192-    * including the possibility of unfinished validation.
193-    */
194+    /** Used to indicate the results of mempool validation. */
195     enum class ResultType {

JeremyRubin commented at 9:39 pm on April 14, 2021:

nit: can inherit from u8 to make smaller

JeremyRubin commented at 9:40 pm on April 14, 2021:

make sure to sort the m_result_type after m_state for packing rules.

glozow commented at 9:13 pm on April 15, 2021:

Most compilers will byte-align it, no?

JeremyRubin commented at 9:17 pm on April 15, 2021:

unclear to me what exact size TxValidationState is.

General rule of thumb unless you know something about accesses occurring together often is to sort in order biggest field to smallest, it guarantees good packing IIRC.

in src/txmempool.cpp:154 in cc00a859de outdated

150@@ -150,33 +151,48 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashes
151     }
152 }
153 
154-bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents /* = true */) const
155+bool CTxMemPool::CalculateMemPoolAncestors(const std::vector<CTxMemPoolEntry>& entries,

JeremyRubin commented at 10:05 pm on April 14, 2021:

Since you’re using a const vec ref here, may as well use the Span API.

This way you can construct a span from a single pointer to a CTxMemPoolEntry and avoid needing to allocate to call CalculateMempoolAncestors

in src/txmempool.cpp:187 in cc00a859de outdated

200-        // If we're not searching for parents, we require this to be an
201-        // entry in the mempool already.
202-        txiter it = mapTx.iterator_to(entry);
203-        staged_ancestors = it->GetMemPoolParentsConst();
204+        // If we're not searching for parents, we require all entries to be in the mempool already.
205+        for (const auto& entry : entries) {

JeremyRubin commented at 10:09 pm on April 14, 2021:

If you’re touching this code you could update it to use Epoch Algorithms.

Essentially all you need to do is get a new epoch, and then visit the txiters you are interested in and add them to a vec instead of a set.

This lets you get rid of the O(n^2) potential of a merge call.

JeremyRubin commented at 10:09 pm on April 14, 2021:

Alternatively, I can PR the epoch algorithm and you can rebase on it, or I can PR it before this code gets released.

glozow commented at 9:14 pm on April 15, 2021:

Alternatively, I can PR the epoch algorithm and you can rebase on it, or I can PR it before this code gets released.

Both sound like good options!

in src/txmempool.cpp:225 in cc00a859de outdated

221@@ -205,7 +222,7 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntr
222             if (setAncestors.count(parent_it) == 0) {
223                 staged_ancestors.insert(parent);
224             }
225-            if (staged_ancestors.size() + setAncestors.size() + 1 > limitAncestorCount) {
226+            if (staged_ancestors.size() + setAncestors.size() + total_count > limitAncestorCount) {

JeremyRubin commented at 10:19 pm on April 14, 2021:

I think staged ancestors is possibly overcounting here, need to run through if there are circumstances where we’d trigger early here.

glozow commented at 10:25 pm on April 14, 2021:

Yes, it’s certainly overestimating for every case except for a package of parent + child. But I assume that’s the vast majority of cases for a package.

~I did just notice that we’re double-counting the current tx though, so I’ll update to do staged_ancestors.size() + setAncestors.size() + total count - 1 > limitAncestorCount~

in src/txmempool.cpp:240 in cc00a859de outdated

231@@ -215,6 +232,13 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntr
232     return true;
233 }
234 
235+bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents /* = true */) const
236+{
237+    return CalculateMemPoolAncestors(std::vector<CTxMemPoolEntry>{entry}, setAncestors,

JeremyRubin commented at 10:21 pm on April 14, 2021:

When you do the span you’d do std::span(&entry, 1) I think

in src/validation.h:219 in cc00a859de outdated

246+    /**
247+    * Map from txid to finished MempoolAcceptResults. The client is responsible
248+    * for keeping track of the transaction objects themselves. If a result is not
249+    * present, it means validation was unfinished for that transaction.
250+    */
251+    std::map<const uint256, const MempoolAcceptResult> m_tx_results;

JeremyRubin commented at 10:49 pm on April 14, 2021:

Can this not be a vector of equal length to the package sent and index it by index?

This will most likely save memory & less work.

glozow commented at 7:52 pm on April 15, 2021:

I guess that’d need to be a vector of optionals - probably more memory, actually. I only insert a result into the map if it’s fully validated, so sometimes it only has 1 result.

JeremyRubin commented at 7:57 pm on April 15, 2021:

Well so the max size is a vec of size 25 – so it’s a single allocation when you reserve v.s. 25 allocations for the map.

Further, the map each allocation is IIRC 2-or-3 pointers (16 - 24 bytes), 32 bytes for hash, and then however big MempoolAcceptResult is.

Further in the case where you do have a few things it’s extra work/indirection to iterate/lookup.

You don’t need a optional because MempoolAcceptResult can use it’s inner enum to have an “uninit” state.

The expected case is that all the txns fully validate right? Wouldn’t we disconnect peers that send us things that don’t validate a lot?

JeremyRubin commented at 8:02 pm on April 15, 2021:

I guess to be more concrete – allocating 25 kinda small things is a vec as single allocation isn’t really an issue, so we might as well do it. In the worst case it’s more efficient, and in the best case it’s not too bad.

in src/validation.cpp:482 in cc00a859de outdated

477+{
478+    /**
479+    * Coins made available by transactions being validated. Tracking these allows for package
480+    * validation, since we can access transaction outputs without submitting them to mempool.
481+    */
482+    std::map<COutPoint, Coin> m_temp_added;

JeremyRubin commented at 10:54 pm on April 14, 2021:

maybe bench using unordered_set here

glozow commented at 9:43 pm on April 16, 2021:

changed to unordered_map

in src/validation.cpp:488 in cc00a859de outdated

483+
484+    /**
485+    * Coins spent by transactions being validated. When validating a package, we need to track
486+    * these in order to distinguish between missing/spent coins and conflicts within a package.
487+    */
488+    std::set<COutPoint> m_temp_spent;

JeremyRubin commented at 11:18 pm on April 14, 2021:

ibid

glozow commented at 9:43 pm on April 16, 2021:

changed to unordered_set

in src/validation.cpp:473 in cc00a859de outdated

486+    * these in order to distinguish between missing/spent coins and conflicts within a package.
487+    */
488+    std::set<COutPoint> m_temp_spent;
489+
490+    /** An empty coin used as a placeholder for a spent coin.*/
491+    inline static const Coin s_coin_spent;

JeremyRubin commented at 11:21 pm on April 14, 2021:

nit: Hmmm… I know GetCoin is perf sensitive but do we do this placeholder coin often?

Would optional coin be cleaner?

glozow commented at 7:55 pm on April 15, 2021:

We had a conversation about the Coins API being a little weird earlier: #20833 (review)

In any case, it’s out of the scope of this PR to change the Coins API, so I’m going to leave it as is :shrug:

in src/validation.cpp:523 in cc00a859de outdated

518+        }
519+        return CCoinsViewCache::AccessCoin(outpoint);
520+    }
521+
522+    bool HaveCoin(const COutPoint& outpoint) const override {
523+        Coin coin;

JeremyRubin commented at 11:25 pm on April 14, 2021:

nit: would be shorter to write !AccessCoin(outpoint).IsSpent()

glozow commented at 9:01 pm on April 16, 2021:

Done

in src/validation.cpp:533 in cc00a859de outdated

528+    * Update with coins spent and created by a transaction.
529+    * Only used for package validation.
530+    */
531+    void PackageAddTransaction(const CTransactionRef& tx)
532+    {
533+        // Track Coins spent by this transaction. They must exist and not already be spent.

JeremyRubin commented at 11:40 pm on April 14, 2021:

this should be documented in the function doc that this must be true.

glozow commented at 9:01 pm on April 16, 2021:

Done

in src/validation.cpp:536 in cc00a859de outdated

531+    void PackageAddTransaction(const CTransactionRef& tx)
532+    {
533+        // Track Coins spent by this transaction. They must exist and not already be spent.
534+        for (auto input : tx->vin) {
535+            Coin spent_coin;
536+            Assume(GetCoin(input.prevout, spent_coin));

JeremyRubin commented at 11:41 pm on April 14, 2021:

HaveCoin?

glozow commented at 9:02 pm on April 16, 2021:

Yeah, that’s shorter - done.

in src/validation.cpp:534 in cc00a859de outdated

557+    void ClearTemporarySpends() {
558+        m_temp_spent.clear();
559+    }
560+
561+    // A CoinsViewTemporary is for temporary scratch space only; it should not write to its backend.
562+    bool Flush() override {

JeremyRubin commented at 0:40 am on April 15, 2021:

I think you should just return false here maybe? would need to check the API but that should tell you that it failed (otherwise change the API to be nothrow)

glozow commented at 5:07 pm on April 16, 2021:

The API says false = the flush went wrong somehow. I’d like to throw because nobody should be trying to flush the temporary coins cache

in src/validation.cpp:1237 in cc00a859de outdated

1239+        Workspace workspace(tx);
1240+        for (const auto& input : tx->vin) {
1241+            if (std::find_if(workspaces.begin(), workspaces.end(),
1242+

rpc/validation: enable packages through testmempoolaccept #20833

Conflicts