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+                             [&, txid = input.prevout.hash](const auto& preceding_ws)
1243+                             { return preceding_ws.m_ptx->GetHash() == txid; }) == workspaces.end() &&
1244+                std::find_if(txns.begin(), txns.end(),

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

would prolly be faster to build a hashset as you go thru here.

If not, you can transform the top for loop to scan the txiters and just scan the txns txids after the current one for a small performance bump.

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

Done, doing a hashset. There’s only one find_if now

in src/validation.cpp:1137 in cc00a859de outdated

1247+                // If we don't find the parent in workspaces but we do find it in txns,
1248+                // then it must be a subsequent transaction in the package.
1249+                package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted");
1250+                return PackageMempoolAcceptResult(package_state, {});
1251+            }
1252+        }

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

note that this is not canonically ordered.

One way to do this is to add a new field “rank” in the workspace. For each tx, it’s rank is max(parent rank) + 1. Therefore no tx has a lower rank than it’s parent. Then you can verify sorted by rank primary and secondary hash a.rank < b.rank || (a.rank == b.rank && a.hash < b.hash).

This lets you verify that you have received the package in canonical order. This is useful if you later want package IDs.

To generate a canonically ordered package, you can sort by hash, and then stable sort by rank.

edit: for correctness

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

To do this change I’d get rid of the find_if’s and make them for loops it will be easier to code.

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

note that this is not canonically ordered.

Note that there is no canonical ordering defined yet 😛 although that’s indeed what I would design the ordering to be. At this point, since this is just for users to test multiple transactions, there’s not really a reason to require this order - i’m only checking the order because it’s faster and a better error message than missing-inputs. Same with package IDs - i think that makes sense for a later PR that defines P2P packages.

JeremyRubin commented at 3:26 am on April 17, 2021:

yep, just a note :)

in src/validation.cpp:1245 in cc00a859de outdated

1248+                // then it must be a subsequent transaction in the package.
1249+                package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted");
1250+                return PackageMempoolAcceptResult(package_state, {});
1251+            }
1252+        }
1253+        workspaces.emplace_back(std::move(workspace));

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

nit: you can directly construct the workspace on the back of workspaces

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

altho then you want to go to end-1 if you do that.

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

what’s end-1?

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

constructing in place done

in src/validation.cpp:1144 in cc00a859de outdated

1256+    LOCK(m_pool.cs);
1257+
1258+    std::map<const uint256, const MempoolAcceptResult> results;
1259+    // Do all PreChecks first and fail fast to avoid running expensive script checks when unnecessary.
1260+    for (Workspace& ws : workspaces) {
1261+        if (!PreChecks(args, ws)) {

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

separately – is it true that PreChecks is not entirely stateless? On edge condition, 2 separate ATMPs could have different minfees & therefore this package code is a little different than separate submissions.

glozow commented at 1:17 pm on April 15, 2021:

PreChecks is not stateless, no. Maybe you’re thinking of CheckTransaction? There’s no way to make separate mempool accepts identical unless we hold cs_main the entire time - you can just as well have different results from the inputs getting spent in between calls.

JeremyRubin commented at 1:30 am on April 15, 2021: contributor

Concept ACK and approach ack.

code looks pretty decent, didn’t do a fine tooth combed review yet though.

glozow force-pushed on Apr 16, 2021

DrahtBot added the label Needs rebase on Apr 17, 2021

glozow force-pushed on Apr 17, 2021

glozow commented at 5:06 pm on April 17, 2021: member

Rebased. Looking into the fuzzer issue

DrahtBot removed the label Needs rebase on Apr 17, 2021

in src/validation.cpp:1228 in b93442473f outdated

1228+        return PackageMempoolAcceptResult(package_state, {});
1229+    }
1230+
1231+    std::vector<Workspace> workspaces{};
1232+    workspaces.reserve(package_count);
1233+    std::unordered_set<uint256, BlockHasher> txids;

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

SaltedTXIDHasher; blockhasher is insecure

glozow commented at 9:09 pm on April 19, 2021:

Ah that is much more appropriate, switched. Not sure what you mean by security in this context, it’s just being used for faster lookup…?

in src/validation.cpp:1237 in b93442473f outdated

1237+    // fail on something less ambiguous (missing-inputs could also be an orphan or trying to
1238+    // spend nonexistent coins).
1239+    for (const auto& tx : txns) {
1240+        for (const auto& input : tx->vin) {
1241+            if (txids.find(input.prevout.hash) == txids.end() &&
1242+                std::find_if(txns.begin(), txns.end(),

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

suggestion:

make std::unordered_map<uint256, bool, SaltedTXIDHasher>, fill it with all the txns in the package set to false, and set true when it’s been passed.

Then when you’re scanning:

0auto it = txids.find(input.prevout.hash);
1if (it == txids.end()) continue;
2if (it->second == false) /* invalid /*;
3/*
4valid
5*/

If you want to get extra fancy to allocate less memory, this is a tmp struct that lives as long as txids so we can just do ptrs

0std::unordered_map<const *CTransaction, bool, SaltedTxidHasher>

where you patch:

 0 class SaltedTxidHasher
 1 {
 2 private:
 3     /** Salt */
 4     const uint64_t k0, k1;
 5 
 6 public:
 7     SaltedTxidHasher();
 8 
 9     size_t operator()(const uint256& txid) const {
10         return SipHashUint256(k0, k1, txid);
11     }
12     size_t operator()(const CTransaction*& tx) const {
13         return SipHashUint256(k0, k1, tx->GetHash());
14     }
15 };

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

Changed to a std::unordered_map<uint256, bool, SaltedTxidHasher> and limited the scope of the map Re: overloading SaltedTxidHasher, I think I’d also want a SaltedWtxidHasher 🤔

JeremyRubin commented at 0:32 am on April 25, 2021:

That shouldn’t matter since they’re immutable in this context :)

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

What’s immutable?

glozow force-pushed on Apr 19, 2021

in src/validation.cpp:778 in 9ce7b0f385 outdated

774 
775                 setConflicts.insert(ptxConflicting->GetHash());
776             }
777         }
778+        // Check for conflicts with transactions in the same package.
779+        if (m_view.PackageSpends(txin.prevout)) {

ariard commented at 5:46 pm on April 22, 2021:

I think this check is context-free and can be checked before taking the mempool lock ? Like at the same time we do package limits checks.

IIRC PackageSpends verifies the lack of outpoint spends identity among elements of the package set but not among the elements of the mempool set.

glozow commented at 5:03 pm on April 26, 2021:

Good point, moved this to the beginning of AcceptMultipleTransactions() before taking the mempool lock

in src/validation.h:204 in 9ce7b0f385 outdated

230 
231     /** Constructor for success case */
232-    explicit MempoolAcceptResult(std::list<CTransactionRef>&& replaced_txns, CAmount fees)
233-        : m_result_type(ResultType::VALID), m_state(TxValidationState{}),
234+    explicit MempoolAcceptResult(std::list<CTransactionRef>&& replaced_txns, const CAmount fees)
235+        : m_result_type(ResultType::VALID),

ariard commented at 5:53 pm on April 22, 2021:

Thanks for the success/failure constructor split since lastest review :)

I think you can add a Assume(state.IsValid() in success case.

jnewbery commented at 7:53 am on April 23, 2021:

There’s no state parameter in the success case.

in src/txmempool.cpp:205 in 9ce7b0f385 outdated

199@@ -186,10 +200,13 @@ bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntr
200         staged_ancestors.erase(stage);
201         totalSizeWithAncestors += stageit->GetTxSize();
202 
203-        if (stageit->GetSizeWithDescendants() + entry.GetTxSize() > limitDescendantSize) {
204+        // When multiple transactions are passed in, the ancestors and descendants of all transactions
205+        // considered together must be within limits even if they are not interdependent. This may be
206+        // stricter than the limits for each individual transaction.

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

IIUC, let’s say you have transaction A and transaction B without dependency between them. If transaction A’s ancestor=20 and transaction B’s ancestor=15, they will be rejected as the union of their ancestors set is beyond the default limitAncestorCount. But if they’re submitted through sequential calls to testmempoolaccept they would be accepted ?

I think this quirk is why I would prefer to disallow submission of independent groups of transactions. If the user is interested by the acceptance of N independent groups of transactions, what’s the concerns of doing N calls to testmempoolaccept ?

Previous discussion : https://github.com/bitcoin/bitcoin/pull/20833/#discussion_r594527279

JeremyRubin commented at 0:34 am on April 25, 2021:

I agree – it should be one connected component. The only “quirk” of this is that “packages” can get “invalidated” after a block is mined.

glozow commented at 5:31 pm on April 26, 2021:

Distinct testmempoolaccept calls returning different results for policy reasons is not a “quirk,” it’s just a natural part of mempool policy. With single test accepts today you could call it 3 times and get 3 different results for the same tx because anything can happen in between calls.

I agree that p2p packages shouldn’t have independent transactions, but we’re not really there yet. This PR is just adding dry-run logic reachable through RPC, and we’ll write docs to make it clear what this should be used for (i.e. not for batch validation). Obviously I wouldn’t want to create a footgun for users who misunderstand/don’t read the release notes (which is why I set a limit of 25 for example), but I don’t think that’s the case here.

TLDR: I agree conceptually, but don’t think it’s necessary in this PR. I’m trying to keep this as bare-bones as possible and it’s 1000+ lines…

ariard commented at 6:25 pm on April 27, 2021:

Distinct testmempoolaccept calls returning different results for policy reasons is not a “quirk,” it’s just a natural part of mempool policy. With single test accepts today you could call it 3 times and get 3 different results for the same tx because anything can happen in between calls.

And I don’t agree with this assumption of anything can happen in between calls. Ultimately you can isolate and disconnect your node to verify the correctness equivalence of testmempoolaccept of package A of size N compare to N sequential call to testmempoolaccept for each member of package A.

That said what previous discussions about RBF conflict sounds to have establish is package policy checks != mempool policy checks, so I don’t think we need to agree on assumptions there. We’ll make it clear to the user that you can succeed a package A but individually fail transaction member or vice-versa.

TLDR: I agree conceptually, but don’t think it’s necessary in this PR. I’m trying to keep this as bare-bones as possible and it’s 1000+ lines…

IIRC, it was on my push to add ancestors/descendants at the package-level but given the complexity in practice I would prefer not to touch txmempool.cpp in this PR. I think we can roll-back e7aa139 and introduce in its own PR ? That would make more happy reviewer to not have think about what we might breack by modifying CalculateMemPoolAncestors :)

glozow commented at 10:56 pm on April 27, 2021:

I think we can roll-back e7aa139 and introduce in its own PR ? That would make more happy reviewer to not have think about what we might breack by modifying CalculateMemPoolAncestors :) @ariard Yeah, good point! Without the ancestor/descendant logic I think we could end up with a chain of 49 at least, but we never actually submit them to mempool here so it won’t really hurt us… I’ve just reordered the commits so everything past the release notes could go in a followup PR, maybe that’d help?

glozow commented at 7:40 pm on April 28, 2021:

separated to #21800

in src/packages.h:16 in 9ce7b0f385 outdated

11+#include <primitives/transaction.h>
12+
13+/** Default maximum number of transactions in a package. */
14+static constexpr uint32_t MAX_PACKAGE_COUNT{25};
15+/** Default maximum total virtual size of transactions in a package in KvB. */
16+static constexpr uint32_t MAX_PACKAGE_SIZE{101};

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

What’s the rational of using KvB instead of weight unit like for MAX_STANDARD_TX_WEIGHT ? I think it would be the first variable in the codebase using virtual kilo-bytes.

glozow commented at 0:09 am on April 23, 2021:

AFAIK the descendant and ancestor limits are denominated in kilo virtual bytes too, no?

darosior commented at 9:06 am on April 23, 2021:

Yes, there are plenty of policy constants in virtual bytes. However they can eventually lead to clumsy behaviour (see #13283).

And also a very slight divergence between the state of our mempool (in virtual bytes) and what miner care about (in weight units) which i’m unsure if we care about (but planned to look into soon :tm: forever).

In any case it seems simply better to have constants in weight units? EDIT: but it would differ with the current behaviour, so i’m not sure actually…

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

the descendant and ancestor limits are denominated in kilo virtual bytes too, no?

Yes:

https://github.com/bitcoin/bitcoin/blob/4b5659c6b115315c9fd2902b4edd4b960a5e066e/src/validation.h#L65-L66

ariard commented at 11:54 pm on April 24, 2021:

I think this project is aiming toward better support of second-layer applications. Given a lot of them are relying on trustless chain of transactions and those ones are necessarily made of segwit inputs, they’re already using weight units, contrary to “legacy” applications.

Sticking to virtual bytes would make such L2 backends worst as you’ll need to scale down your package to the vb unit to ensure you’re respecting the limits.

That said, I don’t think we need to decide now, we all agree the package API is unstable and we can change this back latter in its own PR. Current one is already a good move forward.

Tracking this point in #14895.

glozow commented at 5:35 pm on April 26, 2021:

That said, I don’t think we need to decide now, we all agree the package API is unstable and we can change this back latter in its own PR. Current one is already a good move forward.

Right, this is 100% flexible 👍 will be sure to follow the discussion

What do you mean by “scale down your package?” I didn’t think there would be a significant difference between using weight units and vbytes, even if there’s a bit of rounding

in src/packages.h:6 in 9ce7b0f385 outdated

0@@ -0,0 +1,33 @@
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

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

Good idea to start isolating package policy logic in its own file. I think it’s a good candidate to policy/ ?

glozow commented at 5:03 pm on April 26, 2021:

Sure, moved to src/policy/packages.h

ariard commented at 6:33 pm on April 22, 2021: member

Okay, submission of independent groups of transaction is the last concern I’ve about the API, otherwise I think it’s good. Still reviewing code modularity/test coverage.

in src/validation.h:217 in 9ce7b0f385 outdated

221 
222+// Private constructors. Use static methods MempoolAcceptResult::Success, etc. to construct.
223+private:
224     /** Constructor for failure case */
225-    explicit MempoolAcceptResult(TxValidationState state)
226+    explicit MempoolAcceptResult(const TxValidationState state)

jnewbery commented at 7:52 am on April 23, 2021:

No need to const pass-by-value parameters. Here and CAmount fees below.

in src/validation.cpp:297 in 9ce7b0f385 outdated

294@@ -294,7 +295,6 @@ bool CheckSequenceLocks(CChainState& active_chainstate,
295     }
296     else {
297         // CoinsTip() contains the UTXO set for active_chainstate.m_chain.Tip()

jnewbery commented at 8:24 am on April 23, 2021:

This comment is no longer valid.

glozow commented at 5:04 pm on April 26, 2021:

fixed

in src/validation.cpp:267 in 9ce7b0f385 outdated

264@@ -263,14 +265,13 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp)
265 }
266 
267 bool CheckSequenceLocks(CChainState& active_chainstate,

jnewbery commented at 8:48 am on April 23, 2021:

CheckSequenceLocks() no longer actually requires active_chainstate. You could just as easily pass in a const reference to m_active_chainstate.m_chain.tip and then use that in the function.

glozow commented at 5:04 pm on April 26, 2021:

Good point, I’m having it take a CBlockIndex* now

in src/validation.h:292 in 9ce7b0f385 outdated

288@@ -246,6 +289,8 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE
289 
290 /**
291  * Check if transaction will be BIP 68 final in the next block to be created.
292+ * @param[in]   viewMemPool     A CoinsView that provides access to relevant coins for

jnewbery commented at 8:49 am on April 23, 2021:

It seems slightly odd to call this viewMemPool if any CoinsView can be passed in. Why not just coins_view or view?

glozow commented at 5:05 pm on April 26, 2021:

no reason other than wanting to keep the diff smol, changed the name to coins_view

in src/test/txvalidation_tests.cpp:88 in 9ce7b0f385 outdated

83+    // Packages can't have a total size of more than 101KvB.
84+    CTransactionRef large_ptx = create_placeholder_tx(150, 150);
85+    Package package_too_large;
86+    auto size_large = GetVirtualTransactionSize(*large_ptx);
87+    size_t total_size{0};
88+    while (total_size < MAX_PACKAGE_SIZE * 1000) {

darosior commented at 9:17 am on April 23, 2021:

nit: even though i don’t think it can be hit, you want <= MAX_PACKAGE_SIZE here cause the check in validation is:

0total_size > MAX_PACKAGE_SIZE * 1000

glozow commented at 5:04 pm on April 26, 2021:

Good point! fixed

in src/validation.cpp:529 in 9ce7b0f385 outdated

524+    bool HaveCoin(const COutPoint& outpoint) const override {
525+        return !AccessCoin(outpoint).IsSpent();
526+    }
527+
528+    /**
529+    * Update with coins spent and created by a transaction. All inputs should exist and not already spent.

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

0    * Update with coins spent and created by a transaction. All inputs should exist and not already be spent.

in src/validation.cpp:541 in 9ce7b0f385 outdated

565+
566+    bool BatchWrite(CCoinsMap &mapCoins, const uint256 &hashBlockIn) override {
567+        throw std::logic_error("CoinsViewTemporary writing is not supported.");
568+    }
569+
570+};

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

0    }
1};

in src/validation.h:206 in 9ce7b0f385 outdated

209-    std::optional<CAmount> m_base_fees;
210+    const std::optional<std::list<CTransactionRef>> m_replaced_transactions;
211+    /** Raw base fees in satoshis. */
212+    const std::optional<CAmount> m_base_fees;
213+
214+    static MempoolAcceptResult Failure(const TxValidationState state) {

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

No need for const pass-by-value params.

in src/validation.cpp:1270 in 9ce7b0f385 outdated

1271+        }
1272+        m_view.PackageAddTransaction(ws.m_ptx);
1273+    }
1274+
1275+    // We don't want to modify the workspace entries and won't use the set of ancestors returned.
1276+    std::vector<CTxMemPoolEntry> dummy_entries;

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

I don’t think dummy_entries and dummy_ancestors are the right names here. They really are the entries and ancestors. I’d suggest entries and ancestors_unused, or perhaps entries_copy and ancestors_unused.

in src/validation.cpp:1278 in 9ce7b0f385 outdated

1279+    CTxMemPool::setEntries dummy_ancestors;
1280+    std::string err_string;
1281+    if (!m_pool.CalculateMemPoolAncestors(dummy_entries, dummy_ancestors, m_limit_ancestors,
1282+                                          m_limit_ancestor_size, m_limit_descendants,
1283+                                          m_limit_descendant_size, err_string)) {
1284+        // All transactions must have individually passed mempool ancestor and dsecendant limits

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

0        // All transactions must have individually passed mempool ancestor and descendant limits

in src/validation.cpp:1305 in 9ce7b0f385 outdated

1306+            package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed");
1307+            results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
1308+            return PackageMempoolAcceptResult(package_state, std::move(results));
1309+        }
1310+        // When test_accept=true, transactions that pass PolicyScriptChecks are valid because there are
1311+        // no further mempool checks (passing PolicyScriptChecks implies passing ConsensusScriptChecks).

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

Perhaps expand this comment to say that we’re not calling ConsensusScriptChecks because this function is currently only for testaccept. I think we’d want to update it if we allowed package acceptance to actually add txs to the mempool.

glozow commented at 5:08 pm on April 26, 2021:

True. I’ve updated it to be gated on if(test_accept) so that this code block would still be correct with real package accept.

in src/rpc/rawtransaction.cpp:1000 in 9ce7b0f385 outdated

1022+            // Check that fee does not exceed maximum fee
1023+            const int64_t virtual_size = GetVirtualTransactionSize(*tx);
1024+            const CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size);
1025+            if (max_raw_tx_fee && fee > max_raw_tx_fee) {
1026+                result_inner.pushKV("allowed", false);
1027+                result_inner.pushKV("reject-reason", "max-fee-exceeded");

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

This is a slightly odd interface for the user. Imagine a package of {A, B} is submitted:

B spends one of A’s outputs.
A is valid but has feerate > max_feerate
B is valid and has acceptable feerate

Then the result will show that A is rejected, but B is accepted. That’s different from other failure modes where B won’t be included in the results since it depends on a rejected tx.

Perhaps it’s better to push the failure to rpc_result and then break, so that subsequent transactions don’t show as valid?

glozow commented at 5:09 pm on April 26, 2021:

Oof, yeah that’s super weird… Not sure if it’d be better to break, though, because then we’d be missing B altogether 🤔 will think about this more

mzumsande commented at 9:17 pm on May 10, 2021:

Also, would it make sense to apply the maxfeerate limit on a combined package level, and not for each transaction separately?

ariard commented at 9:04 pm on May 21, 2021:

@mzumsande

I think applying the maxfeerate on a combined package level makes sense once we start to verify mempool min feerate at the package level too ? For now, I think I would be even more liberal than John suggestion and only mark the fee excess on the fainting transaction, while documenting the case in RPC field.

I thought about another solution which would be looping first on all transactions to verify if any of them violates the max_raw_tx_fee then mark allowed=false but it leads to the even more confusing result API of attaching a max-fee-exceeded as reject reason to feerate sane transactions…

glozow commented at 3:23 pm on May 24, 2021:

I think this has been addressed - see #20833 (comment)

in test/functional/rpc_packages.py:106 in 9ce7b0f385 outdated

 98+        garbage_tx = node.createrawtransaction([{"txid": "00" * 32, "vout": 5}], {self.address: 1})
 99+        tx = CTransaction()
100+        tx.deserialize(BytesIO(hex_str_to_bytes(garbage_tx)))
101+        testres_bad = node.testmempoolaccept(self.independent_txns_hex + [garbage_tx])
102+        testres_independent_ids = [{"txid": res["txid"], "wtxid": res["wtxid"]} for res in self.independent_txns_testres]
103+        assert_equal(testres_bad, testres_independent_ids + [

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

Maybe add a comment here that the valid transactions only have their txid and wtxid returned because we didn’t complete validation for them (returned immediately after garbage_tx fails prechecks). Conversely, in the test below, we return the full results for the valid transactions because tx_bad_sig only fails in PolicyChecks, after we’ve already completed validation for the good bois.

in test/functional/rpc_packages.py:53 in 9ce7b0f385 outdated

48+        # Create some transactions that can be reused throughout the test. Never submit these to mempool.
49+        self.independent_txns_hex = []
50+        self.independent_txns_testres = []
51+        for _ in range(3):
52+            coin = self.coins.pop()
53+            rawtx = node.createrawtransaction([{"txid" : coin["txid"], "vout" : 0}],

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

minor style comment: PEP8 says no space before colons: https://www.python.org/dev/peps/pep-0008/#pet-peeves

in test/functional/rpc_packages.py:51 in 9ce7b0f385 outdated

35+        node = self.nodes[0]
36+        self.privkeys = [node.get_deterministic_priv_key().key]
37+        self.address = node.get_deterministic_priv_key().address
38+        self.coins = []
39+        # The last 100 coinbase transactions are premature
40+        for b in node.generatetoaddress(200, self.address)[:100]:

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

Maybe matches the comment better:

0        for b in node.generatetoaddress(200, self.address)[:-100]:

in test/functional/rpc_packages.py:84 in 9ce7b0f385 outdated

79+        rawtx = node.createrawtransaction(inputs, outputs)
80+        prevtxs = [{
81+            "txid": parent_txid,
82+            "vout": n,
83+            "scriptPubKey": parent_locking_script,
84+            "amount": value + Decimal("0.0001"),

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

Seems a bit strange (and less maintainable) to have the caller substract this amount, and then add it back here. Can you just use the value of the parent_txid you’re passing in?

glozow commented at 5:10 pm on April 26, 2021:

Changed it to output the value as well, and just do 1 fee deduction inside chain_transaction

in test/functional/rpc_packages.py:245 in 9ce7b0f385 outdated

233+        self.log.info("Check that in-mempool and in-package descendants are calculated properly in packages")
234+        # Top parent in mempool, M1
235+        first_coin = self.coins.pop()
236+        parent_value = (first_coin["amount"] - Decimal("0.0002")) / 2 # Deduct reasonable fee and make 2 outputs
237+        inputs = [{"txid" : first_coin["txid"], "vout" : 0}]
238+        outputs = [{self.address : parent_value}, {ADDRESS_BCRT1_P2WSH_OP_TRUE : parent_value}]

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

Why do you need different scriptpubkeys here? Can you not send both outputs to the same address?

glozow commented at 5:11 pm on April 26, 2021:

Yeah, iirc you can’t have duplicate addresses in outputs.

in test/functional/rpc_packages.py:256 in 9ce7b0f385 outdated

251+        parent_locking_script = parent_tx.vout[0].scriptPubKey.hex()
252+        value = parent_value
253+        txid = parent_txid
254+        for i in range(12):
255+            value -= Decimal("0.0001") # deduct reasonable fee
256+            (tx, txhex, parent_locking_script) = self.chain_transaction(txid, value, 0, parent_locking_script)

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

It’s a little inconsistent that parent is being used in parent_locking_script to refer to each tx as you work down the chain, whereas in parent_tx it only ever refers to the top tx of the graph.

in test/functional/rpc_packages.py:258 in 9ce7b0f385 outdated

253+        txid = parent_txid
254+        for i in range(12):
255+            value -= Decimal("0.0001") # deduct reasonable fee
256+            (tx, txhex, parent_locking_script) = self.chain_transaction(txid, value, 0, parent_locking_script)
257+            txid = tx.rehash()
258+            if i < 11: # M2a... M11a

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

I think you’ve miscounted here. i takes values between 0 and 10 here (11 values) M2a - M11a is 10 values.

glozow commented at 5:12 pm on April 26, 2021:

Right, the comment was wrong. I’ve fixed it now - note that M2b is made before the chain so it’s M3b…M13b

in test/functional/rpc_packages.py:278 in 9ce7b0f385 outdated

273+        txid = tx_child_b.rehash()
274+        for i in range(12):
275+            value -= Decimal("0.0001") # Deduct reasonable fee
276+            (tx, txhex, parent_locking_script) = self.chain_transaction(txid, value, 0, parent_locking_script)
277+            txid = tx.rehash()
278+            if i < 11: # M3b... M12b

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

Again, I think this is miscounted.

I think if you just change this to:

0            if i < 11: # M2b... M12b

and the one above to # M1a... M11a and update the diagram, we’re all good.

in test/functional/rpc_packages.py:294 in 9ce7b0f385 outdated

284+        for txres in testres_too_long:
285+            assert_equal(txres["reject-reason"], "too-long-mempool-chain")
286+
287+        # Clear mempool and check that the package passes now
288+        node.generate(1)
289+        assert all([res["allowed"] for res in node.testmempoolaccept(rawtxs=package_hex)])

jnewbery commented at 12:04 pm on April 23, 2021:

This is excellent!

in test/functional/rpc_packages.py:426 in 9ce7b0f385 outdated

421+                parents_tx.append(tx)
422+                values.append(value)
423+                parent_locking_scripts.append(parent_locking_script)
424+            child_hex = self.create_child_with_parents(parents_tx, values, parent_locking_scripts)
425+            # Package accept should work with the parents in any order (as long as parents come before child)
426+            random.shuffle(package_hex)

jnewbery commented at 12:07 pm on April 23, 2021:

Perhaps shuffle and test acceptance a few times?

glozow commented at 5:12 pm on April 26, 2021:

Added 10 shuffles!

in test/functional/rpc_packages.py:279 in 9ce7b0f385 outdated

429+            assert all([testres["allowed"] for testres in testres_multiple])
430+
431+            testres_single = []
432+            # Test accept and then submit each one individually, which should be identical to package testaccept
433+            for rawtx in package_hex:
434+                testres_single.append(node.testmempoolaccept([rawtx])[0])

jnewbery commented at 12:08 pm on April 23, 2021:

did you mean to compare testres_single to testrun_multiple?

glozow commented at 5:13 pm on April 26, 2021:

Woops, yes. Good catch

in test/functional/rpc_packages.py:321 in 9ce7b0f385 outdated

470+        assert_equal(testres, [
471+            {"txid": tx1.rehash(), "wtxid": tx1.getwtxid()},
472+            {"txid": tx2.rehash(), "wtxid": tx2.getwtxid(), "allowed": False, "reject-reason": "conflict-in-package"}
473+        ])
474+
475+    def test_rbf(self):

jnewbery commented at 12:09 pm on April 23, 2021:

Nice test. You should try calling it :grin:

in test/functional/rpc_packages.py:471 in 9ce7b0f385 outdated

466+        ])
467+
468+        self.log.info("Test conflicting transactions in the same package")
469+        testres = node.testmempoolaccept([signedtx1["hex"], signedtx2["hex"]])
470+        assert_equal(testres, [
471+            {"txid": tx1.rehash(), "wtxid": tx1.getwtxid()},

jnewbery commented at 12:11 pm on April 23, 2021:

Maybe add a comment about why this is incomplete and not rejected.

in src/validation.cpp:1229 in 9ce7b0f385 outdated

1230+    }
1231+
1232+    std::vector<Workspace> workspaces{};
1233+    workspaces.reserve(package_count);
1234+    {
1235+        std::unordered_map<uint256, bool, SaltedTxidHasher> txids_seen;

jnewbery commented at 12:24 pm on April 23, 2021:

You don’t need a map here, you can just use a set and erase as you go:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index 0b429adfae..c72b5eee02 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -1226,9 +1226,9 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
 5     std::vector<Workspace> workspaces{};
 6     workspaces.reserve(package_count);
 7     {
 8-        std::unordered_map<uint256, bool, SaltedTxidHasher> txids_seen;
 9-        std::transform(txns.cbegin(), txns.cend(), std::inserter(txids_seen, txids_seen.end()),
10-                       [](const auto& tx){ return std::make_pair(tx->GetHash(), false); });
11+        std::unordered_set<uint256, SaltedTxidHasher> later_txs;
12+        std::transform(txns.cbegin(), txns.cend(), std::inserter(later_txs, later_txs.end()),
13+                       [](const auto& tx){ return tx->GetHash(); });
14 
15         // Require the package to be sorted in order of dependency, i.e. parents appear before children.
16         // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and
17@@ -1236,18 +1236,15 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
18         // spend nonexistent coins).
19         for (const auto& tx : txns) {
20             for (const auto& input : tx->vin) {
21-                auto it = txids_seen.find(input.prevout.hash);
22-                if (it == txids_seen.end()) {
23-                    // The parent is not in this package.
24-                    continue;
25-                } else if (!it->second) {
26+                auto it = later_txs.find(input.prevout.hash);
27+                if (it != later_txs.end()) {
28                     // The parent is a subsequent transaction in the package.
29                     package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted");
30                     return PackageMempoolAcceptResult(package_state, {});
31                 }
32             }
33-            // Mark this transaction as seen.
34-            txids_seen[tx->GetHash()] = true;
35+            // Remove from later_txs
36+            later_txs.erase(tx->GetHash());
37             workspaces.emplace_back(Workspace(tx));
38         }
39     }

jnewbery commented at 5:10 pm on April 23, 2021:

Or you could reverse iterate and add as you go:

 0    {
 1        std::unordered_set<uint256, SaltedTxidHasher> later_txs;
 2
 3        // Require the package to be sorted in order of dependency, i.e. parents appear before children.
 4        // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and
 5        // fail on something less ambiguous (missing-inputs could also be an orphan or trying to
 6        // spend nonexistent coins).
 7        //
 8        // Iterate through the package backwards, adding the txid to later_txs. If an earlier tx spends
 9        // an output from later_txs, the package is mis-sorted.
10        for (auto txit = txns.crbegin(); txit != txns.crend(); ++txit) {
11            for (const auto& input : (*txit)->vin) {
12                if (auto it = later_txs.find(input.prevout.hash); it != later_txs.end()) {
13                    // The parent is a later transaction in the package.
14                    package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-not-sorted");
15                    return PackageMempoolAcceptResult(package_state, {});
16                }
17            }
18            // Add to later_txs
19            later_txs.insert((*txit)->GetHash());
20        }
21    }
22
23    std::vector<Workspace> workspaces{};
24    workspaces.reserve(package_count);
25
26    std::transform(txns.cbegin(), txns.cend(), std::back_inserter(workspaces), [](const auto& tx) {
27        return Workspace(tx);
28    });

(note that we can’t add to workspace as we iterate backwards, because it’d be reversed and it’s slightly annoying to unreverse it)

JeremyRubin commented at 0:48 am on April 25, 2021:

Clever!

I guess the only reason to prefer the bool version is if we might handle in-package vs out of package parents differently in the future. Absent such a future need, I agree this works nicely!

The reverse iteration version should be a fair bit more efficient because we only need to hash each tx once for the set lookup, rather than 1x to create and 1x to remove, although it’s a bit more complex.

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

Noice 🧠 I’ve done it this way, it’s prettier

jnewbery commented at 12:28 pm on April 23, 2021: member

This is looking really, really good now. The tests especially are very thorough and easy to read. Excellent work!

I’ve left a bunch of comments, but they’re mainly style related.

This will also need a release note explaining the new functionality, and particularly the limitations (ancestor/descendant limits, no RBF, etc).

glozow force-pushed on Apr 26, 2021

adamjonas commented at 1:46 am on April 27, 2021: member

I’m having difficulty tracking down the cause, but one can reproduce the fuzzer crash with this:

0echo 'dXXi9XV1Dv//X3AAAd3gMMCvoUAAAP//AAABCzYBAL//MQDQ' | base64 --decode  > /tmp/a
1FUZZ=tx_pool_standard ./src/test/fuzz/fuzz /tmp/a

Using the added fuzzer from the last commit, I was able to go back to 2f1d035 to trigger a crash (which is where the methods are added).

in src/validation.cpp:598 in 8d95509150 outdated

593     MempoolAcceptResult AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
594 
595+    /**
596+    * Multiple transaction acceptance. Transactions may or may not be interdependent,
597+    * but must not conflict with each other. Parents must come before children if any
598+    * any dependencies exist.

ariard commented at 3:37 pm on April 27, 2021:

nit: twice any

in src/validation.cpp:475 in 8d95509150 outdated

584@@ -494,11 +585,20 @@ class MemPoolAccept
585          */
586         std::vector<COutPoint>& m_coins_to_uncache;
587         const bool m_test_accept;
588+        /** Disable BIP125 RBFing; disallow all conflicts with mempool transactions. */

ariard commented at 3:39 pm on April 27, 2021:

I would say it’s more replacement which are disallowed rather than conflicts. Conflicts are considered as fatal for mempool acceptance.

glozow commented at 7:08 pm on April 28, 2021:

Hm? Transactions would only be replacing if they conflicted with mempool.

ariard commented at 3:10 pm on May 4, 2021:

I think it’s a glossary point here.

IMO, conflict is the fact that transaction A is spending the same utxo as transaction B. Replacement is the fact that transaction A is substituted to transaction B by mempool logic. You can’t really disallow same utxo spending, this is decided by the utxo owner. On the other hand your mempool logic is free to apply whatever replacement policy ?

jnewbery commented at 12:41 pm on May 26, 2021:

Having a boolean for disallow_thing where the double negative disallow_thing == false means “thing is allowed” is probably not as clear as a allow_thing boolean where allow_thing == true means “thing is allowed”.

I also agree that “replacement” is probably better than “conflicts” here.

in src/rpc/rawtransaction.cpp:988 in 8d95509150 outdated

1006+        UniValue result_inner(UniValue::VOBJ);
1007+        result_inner.pushKV("txid", tx->GetHash().GetHex());
1008+        result_inner.pushKV("wtxid", tx->GetWitnessHash().GetHex());
1009+        auto it = validation_result.m_tx_results.find(tx->GetWitnessHash());
1010+        if (it == validation_result.m_tx_results.end()) {
1011+            // Validation unfinished. Just return the txid and wtxid.

ariard commented at 4:41 pm on April 27, 2021:

IIUC, in case of failure due to a package policy check, we only yell back txid/wtxid of each package member but we don’t indicate the reject reason to the user ?

If txns > 1 and !m_txresults.empty() we can return allowed=true, if m_txresults.empty() we can return allowed=false and the rejection reason ? I don’t think we need to extend testmempoolaccept with a new field.

glozow commented at 7:05 pm on April 27, 2021:

Maybe we can just put the PackageValidationResult error message in each one if it fails due to PCKG_POLICY?

glozow commented at 10:49 pm on April 27, 2021:

I’ve added a “package-error” key for the RPC, so it isn’t just blank

in src/validation.h:246 in 8d95509150 outdated

242+    explicit PackageMempoolAcceptResult(const PackageValidationState& state,
243+                                        std::map<const uint256, const MempoolAcceptResult>&& results)
244+        : m_state{state}, m_tx_results(std::move(results)) {}
245+
246+    /** Constructor to create a PackageMempoolAcceptResult from a single MempoolAcceptResult */
247+    explicit PackageMempoolAcceptResult(const uint256& txid, const MempoolAcceptResult& result)

ariard commented at 4:50 pm on April 27, 2021:

nit: wtxid

glozow commented at 10:48 pm on April 27, 2021:

Thanks, definitely more clear 👍

in src/validation.cpp:1206 in 8d95509150 outdated

1207+
1208+    const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0,
1209+                               [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); });
1210+    // If the package only contains 1 tx, it's better to report the policy violation on individual tx size.
1211+    if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) {
1212+        package_state.Invalid(PackageValidationResult::PCKG_POLICY, "too-large");

ariard commented at 5:16 pm on April 27, 2021:

nit: “max-package-size” to dissociate clearly from the tx-level errors.

glozow commented at 7:06 pm on April 27, 2021:

“package-too-large” maybe?

glozow commented at 10:48 pm on April 27, 2021:

i made it “package-too-large”

in src/validation.cpp:1198 in 8d95509150 outdated

1199+    PackageValidationState package_state;
1200+    const unsigned int package_count = txns.size();
1201+
1202+    // These context-free package limits can be checked before taking the mempool lock.
1203+    if (package_count > MAX_PACKAGE_COUNT) {
1204+        package_state.Invalid(PackageValidationResult::PCKG_POLICY, "too-many-transactions");

ariard commented at 5:17 pm on April 27, 2021:

nit: “max-package-limits”/“max-package-width”

glozow commented at 7:06 pm on April 27, 2021:

“package-too-many-transactions” maybe?

in src/validation.cpp:1243 in 8d95509150 outdated

1244+            for (const auto& input : tx->vin) {
1245+                if (inputs_seen.find(input.prevout) != inputs_seen.end()) {
1246+                    // This input is also present in another tx in the package.
1247+                    for (auto& ws : workspaces) {
1248+                        ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "conflict-in-package");
1249+                        results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));

ariard commented at 5:33 pm on April 27, 2021:

I think I would drop emplacement of tx-level policy failure for now even if there I agree that an in-package conflict always implies an in-mempool conflict.

Similar to policy rules being tighter than consensus ones, package policy rules should be tighter than the mempool policy ones ? So following this reasoning, should we only expose the more severe one to the user ? Though if the package is compliant in itself, but failure is at the tx-level of course we should expose the faultive transaction and corresponding reason.

I would say yet another package API open question. A clean hierarchy of errors would be cool.

glozow commented at 10:51 pm on April 27, 2021:

Ok, I’ve dropped this line. Now, if there’s a package-wide error, the results map is empty (which I think is nice, saves space) and testmempoolaccept will include any PackageValidationResult::PCKG_POLICY error to the “package-error” field in the RPC result

glozow commented at 10:52 pm on April 27, 2021:

I think the hierarchy is now: if there’s a PCKG_POLICY error, we don’t return tx-specific errors.

in src/validation.cpp:1083 in 8d95509150 outdated

1197+    AssertLockHeld(cs_main);
1198+
1199+    PackageValidationState package_state;
1200+    const unsigned int package_count = txns.size();
1201+
1202+    // These context-free package limits can be checked before taking the mempool lock.

ariard commented at 6:03 pm on April 27, 2021:

I think we should encapsulate all package policy checks logic in its own module/file as best as we can. validation.cpp is already big enough and that would be better if we want to expose policy checks in more ad hoc tooling.

I did it quickly there : https://github.com/ariard/bitcoin/commit/8503af2ab5e897fd1f37e0af0844fc5e60bb68a1

What do you think about it ? Feel free to take the patch, otherwise I can keep it as a follow-up for #20833

glozow commented at 7:40 pm on April 28, 2021:

Sounds good for a follow-up :)

glozow force-pushed on Apr 27, 2021

MarcoFalke referenced this in commit edf679503c on Apr 28, 2021

glozow force-pushed on Apr 28, 2021

sidhujag referenced this in commit dd929c7eea on Apr 28, 2021

glozow commented at 7:37 pm on April 28, 2021: member

Note to reviewers:

#21783 shaved off the first commit 🍧 I’ve reordered the commits and moved the ancestor/descendant limit stuff to #21800. I feel that this is safe to do because ancestor/descendant limits are designed to protect us from heavy computation due to large families in the mempool. This PR may underestimate during the test accept, but the only way to submit these transactions is one-by-one through ATMP (which enforces the actual ancestor/descendant limits). Thus, this code doesn’t create a risk of the mempool exceeding our normal limits. The only problem is that testmempoolaccept might incorrectly report success because it underestimates ancestors, so I’ve noted that in the release notes.

I’ve also deferred the fuzz test (which asserted identical results between ProcessNewPackage(1 tx) and ATMP(1 tx) since package policies are starting to diverge from normal policy and have been changing a bunch.

tldr: this PR is now smaller, and hopefully easier to review!

in src/test/txvalidation_tests.cpp:74 in 03fdad5ce2 outdated

69+BOOST_FIXTURE_TEST_CASE(package_limits, TestChain100Setup)
70+{
71+    // Packages can't have more than 25 transactions.
72+    LOCK(cs_main);
73+    Package package_too_many;
74+    package_too_many.resize(MAX_PACKAGE_COUNT + 1);

mzumsande commented at 2:44 pm on May 2, 2021:

resize and emplace_back is too much, the package will be of size 52 instead of the intended 26 and have empty elements.

mzumsande commented at 8:47 am on May 3, 2021:

oh, and when I change the size to 10 here (i.e. having 10 null transactions and 10 regular ones) I get a memory access violation, so ProcessNewPackage seems to have a problem with malformed input. This may be related to the fuzzer crashes @adamjonas mentioned above, but I can’t see any older discussion around this topic because github sucks :cry:

glozow commented at 2:52 pm on May 3, 2021:

agh, good catch! I meant to use reserve() 🤦

glozow commented at 5:25 pm on May 3, 2021:

Added checks for txns.empty() and null transactions…. hopefully this covers it.

in src/rpc/rawtransaction.cpp:890 in b0737bf14c outdated

885@@ -885,8 +886,10 @@ static RPCHelpMan sendrawtransaction()
886 static RPCHelpMan testmempoolaccept()
887 {
888     return RPCHelpMan{"testmempoolaccept",
889-                "\nReturns result of mempool acceptance tests indicating if raw transaction (serialized, hex-encoded) would be accepted by mempool.\n"
890-                "\nThis checks if the transaction violates the consensus or policy rules.\n"
891+                "\nReturns result of mempool acceptance tests indicating if raw transaction(s) (serialized, hex-encoded) would be accepted by mempool.\n"
892+                "\nIf multiple transactions are passed in, parents must come before children and package policies apply: the transactions cannot not conflict with any mempool transactions or each other.\n"

mzumsande commented at 3:22 pm on May 2, 2021:

“cannot not” -> “cannot”

glozow force-pushed on May 3, 2021

glozow commented at 5:26 pm on May 3, 2021: member

Thanks @mzumsande! Last push:

Added some checks and tests for empty package, nullptrs, and null transactions
Fixed resize() -> reserve() in #20833 (review)
Added a quick call to ProcessNewPackage in the tx_pool fuzzer. All it does is make sure it doesn’t throw and gets a fully validated result.
Fixed rpc doc typo #20833 (review)
Rebased on master for the p2p_segwit.py fix

in src/test/txvalidation_tests.cpp:80 in d8642c1999 outdated

75+    const auto result_empty = ProcessNewPackage(::ChainstateActive(), *m_node.mempool, package_empty, true);
76+    BOOST_CHECK(result_empty.m_state.IsInvalid());
77+
78+    package_empty.resize(10); // null transactions
79+    const auto result_null_txns = ProcessNewPackage(::ChainstateActive(), *m_node.mempool, package_empty, true);
80+    BOOST_CHECK(result_empty.m_state.IsInvalid());

ariard commented at 4:09 pm on May 4, 2021:

Beware you’re using result_empty instead of result_null_txns here.

Also can be extended a bit

 0diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp
 1index 802ac754d..4ef519b02 100644
 2--- a/src/test/txvalidation_tests.cpp
 3+++ b/src/test/txvalidation_tests.cpp
 4@@ -74,10 +74,14 @@ BOOST_FIXTURE_TEST_CASE(basic_package_tests, TestChain100Setup)
 5     Package package_empty;
 6     const auto result_empty = ProcessNewPackage(::ChainstateActive(), *m_node.mempool, package_empty, true);
 7     BOOST_CHECK(result_empty.m_state.IsInvalid());
 8+    BOOST_CHECK_EQUAL(result_empty.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
 9+    BOOST_CHECK_EQUAL(result_empty.m_state.GetRejectReason(), "package-too-few-transactions");
10 
11     package_empty.resize(10); // null transactions
12     const auto result_null_txns = ProcessNewPackage(::ChainstateActive(), *m_node.mempool, package_empty, true);
13-    BOOST_CHECK(result_empty.m_state.IsInvalid());
14+    BOOST_CHECK(result_null_txns.m_state.IsInvalid());
15+    BOOST_CHECK_EQUAL(result_null_txns.m_state.GetResult(), PackageValidationResult::PCKG_POLICY);
16+    BOOST_CHECK_EQUAL(result_null_txns.m_state.GetRejectReason(), "package-malformed-transaction");
17 
18     // Packages can't have more than 25 transactions.
19     Package package_too_many;

glozow commented at 10:22 pm on May 4, 2021:

I decided to remove this test and just assert this as a precondition at the top of ProcessNewPackage. It would be an error in our own code if we were passing nullptrs….

in src/validation.cpp:1092 in d8642c1999 outdated

1217+    }
1218+
1219+    const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0,
1220+                               [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); });
1221+    // If the package only contains 1 tx, it's better to report the policy violation on individual tx size.
1222+    if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) {

ariard commented at 4:26 pm on May 4, 2021:

Following diff doesn’t seem to break unit tests nor rpc_packages. Should we outlaw package_count == 1 with an assert at the beginning of AcceptMultipleTransactions ?

I think such case is a bug in caller logic.

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index e9e0ba6e6..e243839dd 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -1213,7 +1213,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std::
 5     const int64_t total_size = std::accumulate(txns.cbegin(), txns.cend(), 0,
 6                                [](int64_t sum, const auto& tx) { return sum + GetVirtualTransactionSize(*tx); });
 7     // If the package only contains 1 tx, it's better to report the policy violation on individual tx size.
 8-    if (package_count > 1 && total_size > MAX_PACKAGE_SIZE * 1000) {
 9+    if (total_size > MAX_PACKAGE_SIZE * 1000) {
10         package_state.Invalid(PackageValidationResult::PCKG_POLICY, "package-too-large");
11         return PackageMempoolAcceptResult(package_state, {});
12     }

glozow commented at 10:22 pm on May 4, 2021:

Following diff doesn’t seem to break unit tests nor rpc_packages.

Ok I added a unit test for it. I just added the condition because I felt it would be more helpful to users.

Should we outlaw package_count == 1 with an assert at the beginning of AcceptMultipleTransactions ?

I don’t really see why we should do this right now. It’s useful to have for testing, and I don’t think it poses any issues at the moment.

ariard commented at 4:45 pm on May 10, 2021:

I think package_count==1 is a good hint of internal API AcceptMultipleTransactions misusage and an assert would be cleaner. Though not a strong opinion.

in test/functional/rpc_packages.py:343 in d8642c1999 outdated

338+        self.log.info("Test that packages cannot conflict with mempool transactions, even if a valid BIP125 RBF")
339+        node.sendrawtransaction(signed_replaceable_tx["hex"])
340+        testres_rbf_single = node.testmempoolaccept([signed_replacement_tx["hex"]])
341+        assert testres_rbf_single[0]["allowed"]
342+        testres_rbf_package = node.testmempoolaccept([signed_replacement_tx["hex"]] + self.independent_txns_hex)
343+        assert_equal(testres_rbf_package[0]["reject-reason"], "txn-mempool-conflict")

ariard commented at 5:04 pm on May 4, 2021:

Let’s try also this variant ?

 0diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
 1index 75621ba12..053954847 100755
 2--- a/test/functional/rpc_packages.py
 3+++ b/test/functional/rpc_packages.py
 4@@ -341,6 +341,9 @@ class RPCPackagesTest(BitcoinTestFramework):
 5         assert testres_rbf_single[0]["allowed"]
 6         testres_rbf_package = node.testmempoolaccept([signed_replacement_tx["hex"]] + self.independent_txns_hex)
 7         assert_equal(testres_rbf_package[0]["reject-reason"], "txn-mempool-conflict")
 8+        # Test conflict detection is independent of conflicting transaction order in submitted package
 9+        testres_rbf_package_reverse = node.testmempoolaccept(self.independent_txns_hex + [signed_replacement_tx["hex"]])
10+        assert_equal(testres_rbf_package_reverse[len(self.independent_txns_hex)]["reject-reason"], "txn-mempool-conflict")
11 
12 if __name__ == "__main__":
13     RPCPackagesTest().main()

glozow commented at 0:29 am on May 6, 2021:

Ok, I’ve added a function assert_testres_equal() that shuffles packages before asserting that the testmempoolaccept result matches, and edited the tests to use this function in all cases where the order shouldn’t matter.

in src/test/fuzz/tx_pool.cpp:225 in d8642c1999 outdated

185@@ -186,6 +186,16 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool)
186         RegisterSharedValidationInterface(txr);
187         const bool bypass_limits = fuzzed_data_provider.ConsumeBool();
188         ::fRequireStandard = fuzzed_data_provider.ConsumeBool();
189+
190+        // Make sure ProcessNewPackage on one transaction works and always fully validates the transaction.
191+        // The result is not guaranteed to be the same as what is returned by ATMP.
192+        const auto result_package = WITH_LOCK(::cs_main,
193+                                    return ProcessNewPackage(node.chainman->ActiveChainstate(), tx_pool, {tx}, true));

ariard commented at 5:12 pm on May 4, 2021:

Have you look to actually pass chain of transactions from fuzzed data to ProcessNewPackage ? I would be glad to have enhanced fuzz coverage of package logic, at least in the future.

glozow commented at 9:22 pm on May 4, 2021:

I thought that would make more sense for actual package submission. I’m not sure if it adds much value with test accepts, since the mempool doesn’t change.

glozow force-pushed on May 4, 2021

glozow force-pushed on May 5, 2021

glozow force-pushed on May 6, 2021

glozow commented at 0:35 am on May 6, 2021: member

Recent pushes

Added asserts for the precondition of non-empty packages and no nullptrs in packages (context: #20833 (review) and #20833 (review))
Added a function assert_testres_equal() that shuffles packages before asserting that the testmempoolaccept result matches, edited the tests to use this function in all cases where the order shouldn’t matter (context: #20833 (review)).
Removed the ::ChainstateActive() calls I had accidentally re-introduced in unit tests
Rebased to fix CI issues

in src/rpc/rawtransaction.cpp:969 in 116fc35d2d outdated

 995-            result_0.pushKV("fees", fees);
 996+    CChainState& chainstate = EnsureChainman(node).ActiveChainstate();
 997+    const PackageMempoolAcceptResult validation_result = [&] {
 998+        LOCK(::cs_main);
 999+        if (txns.size() > 1) return ProcessNewPackage(chainstate, mempool, txns, /* test_accept */ true);
1000+        return PackageMempoolAcceptResult(txns[0]->GetWitnessHash(),

mzumsande commented at 12:50 pm on May 7, 2021:

There should be a check for size 0 - currently testmempoolaccept [] segfaults here.

glozow commented at 6:26 pm on May 11, 2021:

Thanks for catching that! Fixed and added a test to mempool_accept.py.

in src/rpc/rawtransaction.cpp:1014 in d93d51f113 outdated

1042+            }
1043         } else {
1044-            result_0.pushKV("reject-reason", state.GetRejectReason());
1045+            result_inner.pushKV("allowed", false);
1046+            const TxValidationState state = accept_result.m_state;
1047+            if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {

ariard commented at 1:32 pm on May 10, 2021:

Do we have any reason to special-case TX_MISSING_INPUTS ?

It sounds to have been here since the introduction of testmempoolaccept with #11742. Though, looking on error path in `PreChecks, L773, the reject reason sounds informative enough “bad-txns-inputs-missingorspent”

glozow commented at 5:15 pm on May 10, 2021:

(Out of the scope of this PR to change this) Correct, I don’t think the special gate on TX_MISSING_INPUTS is necessary anymore, but changing it will mean all the tests will need to assert “bad-txns-inputs-missingorspent” instead of “missing-inputs.”

in src/validation.cpp:1204 in aae6c0b7ef outdated

1203+                       [](const auto& tx) { return tx->GetHash(); });
1204+        // Require the package to be sorted in order of dependency, i.e. parents appear before children.
1205+        // An unsorted package will fail anyway on missing-inputs, but it's better to quit earlier and
1206+        // fail on something less ambiguous (missing-inputs could also be an orphan or trying to
1207+        // spend nonexistent coins).
1208+       for (const auto& tx : txns) {

laanwj commented at 3:17 pm on May 10, 2021:

nit: indentation is off-by-one here (same for the closing bracket below)

glozow commented at 6:26 pm on May 11, 2021:

Oopsie 🤦 fixed now

in doc/release-notes.md:119 in d93d51f113 outdated

114+  contain more than 25 transactions or have a total size exceeding 101K virtual bytes, and the
115+  transactions must be ordered by dependency (parents appear before children).  Transactions cannot
116+  conflict with (spend the same inputs as) each other or the mempool, even if it would be a valid
117+  BIP125 replace-by-fee. There are also limitations on how accurate the policy checks are: a package
118+  that passed `testmempoolaccept` may actually exceed in-mempool ancestor limits or be below the
119+  true minimum mempool fee. (#20833)

ariard commented at 3:57 pm on May 10, 2021:

Thanks for noting the outcome of this conversation, but I think this limitation would be better described as “be rejected as infringing on mempool size limit if effectively added”. What we can describe as min mempool fee enforcement I believe is done by CheckFeeRate.

glozow commented at 5:17 pm on May 10, 2021:

Ok, I can update the doc. Generally, would it be better to just note “multiple transaction testmempoolaccept is experimental, expect the API to be unstable?” And then list the known limitations.

glozow commented at 6:36 pm on May 11, 2021:

Updated

in src/validation.cpp:644 in d93d51f113 outdated

640@@ -541,7 +641,7 @@ class MemPoolAccept
641 
642 private:
643     CTxMemPool& m_pool;
644-    CCoinsViewCache m_view;
645+    CoinsViewTemporary m_view;

ariard commented at 4:17 pm on May 10, 2021:

nit: what about m_viewtmp to dissociate clearly from m_viewmempool?

glozow commented at 6:35 pm on May 11, 2021:

sorree 🙈 there’s no more CoinsViewTemporary

ariard commented at 4:54 pm on May 10, 2021: member

I had a look on my previous reviews and I think all my comments have been addressed or noted for follow-ups (at least the ones not swallowed by GH).

As other comments unresolved I think we have this one, do we have others ?

mzumsande commented at 9:21 pm on May 10, 2021: member

Concept ACK. Not too experienced in validation, but I reviewed the code and played around with the tests and the RPC, and it looks really good to me - some small points below.

in src/rpc/rawtransaction.cpp:954 in d93d51f113 outdated

958-    NodeContext& node = EnsureAnyNodeContext(request.context);
959+    std::vector<CTransactionRef> txns;
960+    for (const auto& rawtx : raw_transactions.getValues()) {
961+        CMutableTransaction mtx;
962+        if (!DecodeHexTx(mtx, rawtx.get_str())) {
963+            throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input.");

laanwj commented at 8:00 am on May 11, 2021:

Would it be useful to report which transaction index failed to decode?

glozow commented at 6:28 pm on May 11, 2021:

Added rawtx.get_str() to the error message, hopefully that would work?

laanwj commented at 1:20 pm on May 17, 2021:

That works too!

in src/rpc/rawtransaction.cpp:971 in d93d51f113 outdated

1001+               AcceptToMemoryPool(chainstate, mempool, txns[0], /* bypass_limits */ false, /* test_accept*/ true));
1002+    }();
1003+
1004+    UniValue rpc_result(UniValue::VARR);
1005+
1006+    for (auto tx : txns) {

laanwj commented at 8:27 am on May 11, 2021:

for (const auto &tx : txns) { maybe ? (no need for mutable copy here)

glozow commented at 6:28 pm on May 11, 2021:

Good point, fixed

in src/test/txvalidation_tests.cpp:61 in d93d51f113 outdated

56+{
57+    CMutableTransaction mtx = CMutableTransaction();
58+    mtx.vin.resize(num_inputs);
59+    mtx.vout.resize(num_outputs);
60+    auto random_script = CScript() << ToByteVector(InsecureRand256()) << ToByteVector(InsecureRand256());
61+    for (size_t i{0}; i < num_inputs; ++i) {

laanwj commented at 8:32 am on May 11, 2021:

nit: indentation on the lines below, as well as the next loop, is off

glozow commented at 6:28 pm on May 11, 2021:

Done, thanks!

glozow force-pushed on May 11, 2021

glozow commented at 6:34 pm on May 11, 2021: member

Bigger changes:

Rolled the “disable RBF” logic into the package accept commit, because it’s a key part of why the logic is correct (mainly just reordering the commits, overall diff is the same).
An offline discussion with @sdaftuar helped me realize that the temporary coins fit more naturally in m_viewmempool. I’ve reverted back to adding a temporary map in CCoinsViewMemPool instead of creating a new CoinsViewTemporary. Also removed m_temp_spent which is no longer necessary since conflicts are checked ahead of time and RBF is disabled from the get-go. This also has the added benefit of simplifying the diff, not touching coins.h, and bloating validation.cpp much less.

Smaller changes:

Added check for empty array passed into testmempoolaccept (also added a test case for that in mempool_accept.py), see #20833 (review)
Fixed indentation issues, use references in for range loop, report which tx fails to decode
Updated the release notes to hopefully be more clear

glozow commented at 7:57 pm on May 11, 2021: member

Github is struggling to load past comments, so I’ve compiled summaries/links for most of the hawt topics of discussion in this gist. It’s not an exhaustive list of everything that’s been discussed, but if you’re wondering “has this already been discussed?” you might find something useful there. I also don’t mind if you write a review comment that’s been said before.

Please note that while this PR is hoping to lay some groundwork for Package Relay, we’re only trying to do test accepts via RPC here :) some discussions might be better for #14895. I appreciate everyone’s feedback! 🤗

in doc/release-notes.md:111 in ff79bbfbdf outdated

107@@ -108,6 +108,15 @@ Updated RPCs
108   Respectively, these new fields indicate the duration of a ban and the time remaining until a ban expires,
109   both in seconds. Additionally, the `ban_created` field is repositioned to come before `banned_until`. (#21602)
110 
111+- The `testmempoolaccept` RPC now accepts multiple transactions (still experimental at the moment,

achow101 commented at 10:01 pm on May 11, 2021:

In d93d51f1132bc6ae54f7d23a5bbb434ce01b7c37 “[doc] add release note for package testmempoolaccept”

nit: Generally release notes are added in a separate file named release-notes-<pr number>.md rather than release-notes.md to avoid causing rebase conflicts for other PRs. So for this PR, you would put this release note in release-notes-20833.md.

glozow commented at 2:21 pm on May 17, 2021:

Ah thank you, noted!

glozow commented at 8:44 pm on May 20, 2021:

Done

in src/validation.h:190 in e77f949d27 outdated

183@@ -185,6 +184,16 @@ struct MempoolAcceptResult {
184     /** Raw base fees in satoshis. */
185     const std::optional<CAmount> m_base_fees;
186 
187+    static MempoolAcceptResult Failure(TxValidationState state) {
188+        return MempoolAcceptResult(state);
189+    }
190+
191+    static MempoolAcceptResult Success(std::list<CTransactionRef>&& replaced_txns, CAmount fees) {

achow101 commented at 10:05 pm on May 11, 2021:

In 0d868d0abbc9655e2e70766433d53719debf27b4 “[validation] package validation for test accepts”

nit: I would prefer for these changes to MempoolAcceptResult be split into a previous commit. It would make this commit slightly easier to read IMO.

glozow commented at 8:44 pm on May 20, 2021:

Done

achow101 commented at 10:10 pm on May 11, 2021: member

Code Review ACK d93d51f1132bc6ae54f7d23a5bbb434ce01b7c37

laanwj approved

laanwj commented at 1:30 pm on May 17, 2021: member

Code review ACK 1bb8fa3a454a8e47972234855efe8f81ab18f355

DrahtBot added the label Needs rebase on May 20, 2021

[validation] make CheckSequenceLocks context-free

Allow CheckSequenceLocks to use heights and coins from any CoinsView and
CBlockIndex provided. This means that CheckSequenceLocks() doesn't need
to hold the mempool lock or cs_main. The caller is responsible for
ensuring the CoinsView and CBlockIndex are consistent before passing
them in. The typical usage is still to create a CCoinsViewMemPool from
the mempool and grab the CBlockIndex from the chainstate tip.

42cf8b25df

[coins/mempool] extend CCoinsViewMemPool to track temporary coins 897e348f59

[refactor] add option to disable RBF

This is a mere refactor for now. We will use this to disable RBFing in
package validation.

249f43f3cc

[policy] Define packages

Define the Package type as an alias for a vector of transactions for now.
Add PackageValidationResult, similar to TxValidationResult and
BlockValidationResult for package-wide errors that cannot be reported
within a single transaction result, such as having too many
transactions in the package. We can update the concept of
what a package is and have different logic for packages vs lists of
transactions in the future, e.g. for package relay.

b88d77aec5

[validation] explicit Success/Failure ctors for MempoolAcceptResult

Makes code more clear and prevents accidentally calling the wrong ctor.

578148ded6

glozow force-pushed on May 20, 2021

glozow commented at 9:59 pm on May 20, 2021: member

Rebased on master, split up a commit into 2 (https://github.com/bitcoin/bitcoin/pull/20833#discussion_r630571069), made a release-notes-20833.md. No other changes.

DrahtBot removed the label Needs rebase on May 20, 2021

in src/validation.cpp:1235 in 930c6d3b12 outdated

1230+    // Ensure the cache is still within its size limits.
1231+    for (const COutPoint& hashTx : coins_to_uncache) {
1232+        active_chainstate.CoinsTip().Uncache(hashTx);
1233+    }
1234+    BlockValidationState state_dummy;
1235+    active_chainstate.FlushStateToDisk(chainparams, state_dummy, FlushStateMode::PERIODIC);

ariard commented at 10:48 pm on May 21, 2021:

For now, any package coins is removed from the cache, so what this call to FlushStateToDisk is achieving ? If this motivated by anticipation of calling PNP for real mempool submissions, at least add a comment saying this is genuine ?

glozow commented at 3:04 pm on May 24, 2021:

Correct, the FlushStateToDisk will not write anything new, so only needed for real submission in the future. I’ve removed this line.

in src/rpc/rawtransaction.cpp:981 in 930c6d3b12 outdated

1017-        result_0.pushKV("allowed", false);
1018-        const TxValidationState state = accept_result.m_state;
1019-        if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
1020-            result_0.pushKV("reject-reason", "missing-inputs");
1021+        auto it = validation_result.m_tx_results.find(tx->GetWitnessHash());
1022+        if (it == validation_result.m_tx_results.end()) {

ariard commented at 10:55 pm on May 21, 2021:

I think it would be clearer to rename validation_result to package_result and accept_result to transaction_result ? Right now it’s a bit confusing what those result are representing.

glozow commented at 3:04 pm on May 24, 2021:

Good idea, I agree that is more clear. I’ve renamed them.

in src/rpc/rawtransaction.cpp:916 in 930c6d3b12 outdated

911                             {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
912                             {RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"},
913-                            {RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"},
914+                            {RPCResult::Type::STR, "package-error", "Package validation error, if any (only possible if rawtxs had more than 1 transaction)."},
915+                            {RPCResult::Type::BOOL, "allowed", "Whether this tx would be accepted to the mempool and passes client-specified maxfeerate.\n"
916+                                                               "If not present, the tx was not fully validated due to a failure in another tx in the list."},

ariard commented at 10:57 pm on May 21, 2021:

What do you think about adding a note like “If a package is submitted and one or more transactions are not passing the check, submission failure is only marked for them and do not propagate to the package” ?

To solve the discussion here : https://github.com/bitcoin/bitcoin/pull/20833/files#r619141139

glozow commented at 3:25 pm on May 24, 2021:

I’ve changed the maxfeerate API a little bit and added “If one transaction fails, other transactions may not be fully validated (the ‘allowed’ key will be blank).” to the RPC description.

ariard commented at 11:10 pm on May 21, 2021: member

Still reviewing, thanks for the changes related to ctors and dropping CoinsViewTemporary, make the PR clearer.

[validation] package validation for test accepts

Only allow test accepts for now. Use the CoinsViewTemporary to keep
track of coins created by each transaction so that subsequent
transactions can spend them. Uncache all coins since we only
ever do test accepts (Note this is different from ATMP which doesn't
uncache for valid test_accepts) to minimize impact on the coins cache.

Require that the input txns have no conflicts and be ordered
topologically. This commit isn't able to detect unsorted packages.

2ef187941d

[test] make submit optional in CreateValidMempoolTransaction

This allows us to easily create transaction chains for package
validation. We don't test_accept if submit=false because we want to be
able to make transactions that wouldn't pass ATMP (i.e. a child
transaction in a package would fail due to missing inputs).

cd9a11ac96

[test] unit tests for ProcessNewPackage

Key functionality = a transaction with UTXOs not present in UTXO set
or mempool can be fully validated instead of being considered an orphan.

363e3d916c

[fuzz] add ProcessNewPackage call in tx_pool fuzzer c9e1a26d1f

[policy] limit package sizes

Maximum number of transactions allowed in a package is 25, equal to the
default mempool descendant limit: if a package has more transactions
than this, either it would fail default mempool descendant limit or the
transactions don't all have a dependency relationship (but then they
shouldn't be in a package together). Same rationale for 101KvB virtual
size package limit.

Note that these policies are only used in test accepts so far.

ae8e6df709

[rpc] allow multiple txns in testmempoolaccept

Only allow "packages" with no conflicts, sorted in order of dependency,
and no more than 25 for now.  Note that these groups of transactions
don't necessarily need to adhere to some strict definition of a package
or have any dependency relationships. Clients are free to pass in a
batch of 25 unrelated transactions if they want to.

9ede34a6f2

[test] functional test for packages in RPCs c4259f4b7e

[doc] add release note for package testmempoolaccept 9ef643e21b

[policy] detect unsorted packages 13650fe2e5

glozow force-pushed on May 24, 2021

glozow commented at 3:22 pm on May 24, 2021: member

Thanks @ariard for reminding me about the max-fee-exceeded API discussion. I’ve just pushed an update similar to @jnewbery’s suggestion in https://github.com/bitcoin/bitcoin/pull/20833/files#r619141139. If we’re satisfied with this API for max-fee-exceeded, then all of the discussions will have been addressed.

My proposal is: If a transaction in the package exceeds maxfeerate, the “reject-reason” is “max-fee-exceeded” and all subsequent transactions have blank validation results (i.e. only “txid” and “wtxid” fields). This is the same as what we do if a transaction fails validation and is still compatible with the API on master.

To respond to @mzumsande’s suggestion in that comment thread:

Also, would it make sense to apply the maxfeerate limit on a combined package level, and not for each transaction separately?

In the future, yes. I plan to open a followup PR to have ProcessNewPackage calculate (and testmempoolaccept return) the descendant feerate and ancestor feerate for each transaction in the package. The intent is to enable the CPFP use case where parent doesn’t meet minimum fee but descendant feerate with child is sufficient. Then, we can apply mempool min fee policy to descendant feerate (so that packages can fee-bump) and enforce the client-specified maxfeerate on the ancestor feerate. This would apply the protection for “I’m trying to fee-bump this transaction’s ancestor(s), but I don’t want to overshoot it,” which is what I believe the user’s intent would be.

in src/validation.cpp:257 in 13650fe2e5

259-    AssertLockHeld(cs_main);
260-    AssertLockHeld(pool.cs);
261-    assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));
262-
263-    CBlockIndex* tip = active_chainstate.m_chain.Tip();
264     assert(tip != nullptr);

jnewbery commented at 12:25 pm on May 26, 2021:

(Doesn’t necessarily need to be in this PR - can be done in a follow-up) it’d be nice to make tip a reference, to better communicate that this function can’t be called without a CBlockIndex.

in src/txmempool.cpp:518 in 13650fe2e5

514@@ -515,7 +515,9 @@ void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags)
515         LockPoints lp = it->GetLockPoints();
516         assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));
517         bool validLP =  TestLockPointValidity(active_chainstate.m_chain, &lp);
518-        if (!CheckFinalTx(active_chainstate.m_chain.Tip(), tx, flags) || !CheckSequenceLocks(active_chainstate, *this, tx, flags, &lp, validLP)) {
519+        CCoinsViewMemPool viewMempool(&active_chainstate.CoinsTip(), *this);

jnewbery commented at 12:26 pm on May 26, 2021:

0        CCoinsViewMemPool view_mempool(&active_chainstate.CoinsTip(), *this);

in src/test/miner_tests.cpp:31 in 13650fe2e5

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(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool);

jnewbery commented at 12:27 pm on May 26, 2021:

0        CCoinsViewMemPool view_mempool(&m_node.chainman->ActiveChainstate().CoinsTip(), *m_node.mempool);

in src/validation.cpp:645 in 13650fe2e5

641@@ -638,7 +642,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
642                         break;
643                     }
644                 }
645-                if (fReplacementOptOut) {
646+                if (fReplacementOptOut || args.disallow_mempool_conflicts) {

jnewbery commented at 12:50 pm on May 26, 2021:

This is equivalent, but I think separating out these reasons is clearer:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index ebe88ba04d..8aff63c4a6 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -619,6 +619,11 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 5     {
 6         const CTransaction* ptxConflicting = m_pool.GetConflictTx(txin.prevout);
 7         if (ptxConflicting) {
 8+            if (args.disallow_mempool_conflicts) {
 9+                // Transaction conflicts with mempool tx, but we're not allowing replacement
10+                return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict");
11+            }
12+
13             if (!setConflicts.count(ptxConflicting->GetHash()))
14             {
15                 // Allow opt-out of transaction replacement by setting
16@@ -642,7 +647,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
17                         break;
18                     }
19                 }
20-                if (fReplacementOptOut || args.disallow_mempool_conflicts) {
21+                if (fReplacementOptOut) {
22                     return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "txn-mempool-conflict");
23                 }
24

Logically, we want to abort mempool acceptance as soon as we encounter a conflict in the mempool if disallow_mempool_conflicts is set to true. Lumping that together with the BIP125 nSequence logic is unnecessary.

ariard commented at 3:37 am on May 27, 2021:

+1, I would also update the reject reason to “txn-mempool-disallowed-replacement”

glozow commented at 4:06 pm on May 27, 2021:

In #22084 I’m updating it to “allow_bip125_replacement” to fix the double-negative and saying “replacements not allowed” instead of “conflicts with mempool.” Also moving the if condition up as suggested.

in src/validation.cpp:1226 in 13650fe2e5

1221+
1222+    std::vector<COutPoint> coins_to_uncache;
1223+    const CChainParams& chainparams = Params();
1224+    MemPoolAccept::ATMPArgs args { chainparams, GetTime(), /* bypass_limits */ false, coins_to_uncache,
1225+                                   test_accept, /* disallow_mempool_conflicts */ true };
1226+    assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));

jnewbery commented at 1:00 pm on May 26, 2021:

I don’t think you need this. These addressof() calls are going to be removed in a future bundle PR. Not adding this line will avoid conflicting with that PR.

in src/validation.cpp:1230 in 13650fe2e5

1225+                                   test_accept, /* disallow_mempool_conflicts */ true };
1226+    assert(std::addressof(::ChainstateActive()) == std::addressof(active_chainstate));
1227+    const PackageMempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptMultipleTransactions(package, args);
1228+
1229+    // Uncache coins pertaining to transactions that were not submitted to the mempool.
1230+    // Ensure the cache is still within its size limits.

jnewbery commented at 1:02 pm on May 26, 2021:

I don’t think the “Ensure the cache is still within its size limits” is relevant here (I think you copied it from the AcceptToMemoryPoolWithTime() function, which does call FlushStateToDisk() to ensure the cache is still within its size limits.

glozow commented at 4:06 pm on May 27, 2021:

Erasing in #22084

in src/validation.cpp:1152 in 13650fe2e5

1153+            results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
1154+            return PackageMempoolAcceptResult(package_state, std::move(results));
1155+        }
1156+        // Make the coins created by this transaction available for subsequent transactions in the
1157+        // package to spend. Since we already checked conflicts in the package and RBFs are
1158+        // impossible, we don't need to track the coins spent. Note that this logic will need to be

jnewbery commented at 1:14 pm on May 26, 2021:

0        // package to spend. Since we already checked conflicts in the package and replacements are
1        // disallowed, we don't need to track the coins spent. Note that this logic will need to be

in src/validation.cpp:1166 in 13650fe2e5

1167+            // Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished.
1168+            package_state.Invalid(PackageValidationResult::PCKG_TX, "transaction failed");
1169+            results.emplace(ws.m_ptx->GetWitnessHash(), MempoolAcceptResult::Failure(ws.m_state));
1170+            return PackageMempoolAcceptResult(package_state, std::move(results));
1171+        }
1172+        if (args.m_test_accept) {

jnewbery commented at 1:42 pm on May 26, 2021:

This if conditional seems unnecessary for this PR. Perhaps just assert that args.m_test_accept is true. A future PR can make the logic here conditional.

glozow commented at 4:42 pm on May 26, 2021:

Right, but by this reasoning, I could also just take the test_accept argument out of this PR altogether. It feels a bit safer to have this condition up front and worry less about gating test-accept-only logic later.

in src/policy/packages.h:13 in 13650fe2e5

 8+#include <consensus/validation.h>
 9+#include <primitives/transaction.h>
10+
11+#include <vector>
12+
13+/** Default maximum number of transactions in a package. */

jnewbery commented at 1:54 pm on May 26, 2021:

Perhaps static assert that these are >= the mempool default ancestor/descendant limits, and that MAX_PACKAGE_SIZE * 4 is >= MAX_STANDARD_TX_WEIGHT

glozow commented at 4:06 pm on May 27, 2021:

adding in #22084

in src/rpc/rawtransaction.cpp:892 in 13650fe2e5

885@@ -885,8 +886,11 @@ static RPCHelpMan sendrawtransaction()
886 static RPCHelpMan testmempoolaccept()
887 {
888     return RPCHelpMan{"testmempoolaccept",
889-                "\nReturns result of mempool acceptance tests indicating if raw transaction (serialized, hex-encoded) would be accepted by mempool.\n"
890-                "\nThis checks if the transaction violates the consensus or policy rules.\n"
891+                "\nReturns result of mempool acceptance tests indicating if raw transaction(s) (serialized, hex-encoded) would be accepted by mempool.\n"
892+                "\nIf multiple transactions are passed in, parents must come before children and package policies apply: the transactions cannot conflict with any mempool transactions or each other.\n"
893+                "\nIf one transaction fails, other transactions may not be fully validated (the 'allowed' key will be blank).\n"
894+                "\nThe maximum number of transactions allowed is 25 (MAX_PACKAGE_COUNT)\n"

jnewbery commented at 2:10 pm on May 26, 2021:

Perhaps use ToString(MAX_PACKAGE_COUNT) here?

glozow commented at 4:06 pm on May 27, 2021:

done in #22084

in src/rpc/rawtransaction.cpp:953 in 13650fe2e5

957     const CFeeRate max_raw_tx_fee_rate = request.params[1].isNull() ?
958                                              DEFAULT_MAX_RAW_TX_FEE_RATE :
959                                              CFeeRate(AmountFromValue(request.params[1]));
960 
961-    NodeContext& node = EnsureAnyNodeContext(request.context);
962+    std::vector<CTransactionRef> txns;

jnewbery commented at 2:46 pm on May 26, 2021:

Perhaps reserve raw_transactions.size() here to avoid reallocations as you emplace the CTransactionRef objects into this vector.

glozow commented at 4:06 pm on May 27, 2021:

done in #22084

in src/validation.cpp:1102 in 13650fe2e5

1103+    // Construct workspaces and check package policies.
1104+    std::vector<Workspace> workspaces{};
1105+    workspaces.reserve(package_count);
1106+    {
1107+        std::unordered_set<uint256, SaltedTxidHasher> later_txids;
1108+        std::transform(txns.cbegin(), txns.cend(), std::inserter(later_txids, later_txids.end()),

jnewbery commented at 3:51 pm on May 26, 2021:

Can you use std::back_inserter() here?

glozow commented at 4:18 pm on May 26, 2021:

IIRC no, because it’s an unordered set.

jnewbery commented at 4:03 pm on May 26, 2021: member

Code review ACK 13650fe2e527bf0cf5d977bf5f3f1563b853ecdc

A few minor comments inline. A couple more:

The commit log for 2ef18794 ([validation] package validation for test accepts) refers to CoinsViewTemporary, which has now been removed.
maybe move the [policy] detect unsorted packages to before the [rpc] allow multiple txns in testmempoolaccept (so you don’t have to add tests and then change them later)

in src/rpc/rawtransaction.cpp:977 in 9ede34a6f2 outdated

1007+
1008+    UniValue rpc_result(UniValue::VARR);
1009+    // We will check transaction fees we iterate through txns in order. If any transaction fee
1010+    // exceeds maxfeerate, we will keave the rest of the validation results blank, because it
1011+    // doesn't make sense to return a validation result for a transaction if its ancestor(s) would
1012+    // not be submitted.

mzumsande commented at 11:22 pm on May 26, 2021:

fees while we iterate, keave->leave

in src/rpc/rawtransaction.cpp:908 in 9ede34a6f2 outdated

905                 },
906                 RPCResult{
907                     RPCResult::Type::ARR, "", "The result of the mempool acceptance test for each raw transaction in the input array.\n"
908-                        "Length is exactly one for now.",
909+                        "Returns results for each transaction in the same order they were passed in.\n"
910+                        "It is possible for transactions to not be fully validated ('allowed' unset) if an earlier transaction failed.\n",

mzumsande commented at 0:16 am on May 27, 2021:

maybe “if another transaction failed”, because it’s not necessarily an earlier transaction? (in the case where validation terminates early, the culprit could also be a later transaction)

in src/rpc/rawtransaction.cpp:915 in 13650fe2e5

913                         {
914                             {RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
915                             {RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"},
916-                            {RPCResult::Type::BOOL, "allowed", "If the mempool allows this tx to be inserted"},
917+                            {RPCResult::Type::STR, "package-error", "Package validation error, if any (only possible if rawtxs had more than 1 transaction)."},
918+                            {RPCResult::Type::BOOL, "allowed", "Whether this tx would be accepted to the mempool and pass client-specified maxfeerate."

mzumsande commented at 0:21 am on May 27, 2021:

nit: space after maxfeerate

glozow commented at 10:03 am on May 27, 2021:

Are you saying I should add a space after maxfeerate or?

mzumsande commented at 10:39 am on May 27, 2021:

I meant after “maxfeerate.” and before the next sentence, it should show when displaying the rpc help.

in src/validation.cpp:484 in 2ef187941d outdated

477@@ -477,6 +478,13 @@ class MemPoolAccept
478     // Single transaction acceptance
479     MempoolAcceptResult AcceptSingleTransaction(const CTransactionRef& ptx, ATMPArgs& args) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
480 
481+    /**
482+    * Multiple transaction acceptance. Transactions may or may not be interdependent,
483+    * but must not conflict with each other. Parents must come before children if any
484+    * dependencies exist, otherwise a TX_MISSING_INPUTS error will be returned.

mzumsande commented at 0:45 am on May 27, 2021:

After the last commit, the error will be “package-not-sorted”.

mzumsande commented at 1:04 am on May 27, 2021: member

I like the way max-fee-exceeded is handled now. Some comments below, feel free to ignore if too nitty.

in src/policy/packages.h:20 in 13650fe2e5

15+/** Default maximum total virtual size of transactions in a package in KvB. */
16+static constexpr uint32_t MAX_PACKAGE_SIZE{101};
17+
18+/** A "reason" why a package was invalid. It may be that one or more of the included
19+ * transactions is invalid or the package itself violates our rules.
20+ * We don't distinguish between consensus and policy violations right now.

ariard commented at 3:47 am on May 27, 2021:

What do you mean by this ? If it’s a PCKG_TX, the yielded TxValidationResult might be TX_CONSENSUS. For e.g receiving a coinbase transaction.

glozow commented at 7:09 am on May 27, 2021:

The “right now” means that we might distinguish between them in the future, e.g. if we want to punish a peer for a PCKG_TX having a consensus failure but not for a policy failure.

in src/rpc/rawtransaction.cpp:891 in 13650fe2e5

885@@ -885,8 +886,11 @@ static RPCHelpMan sendrawtransaction()
886 static RPCHelpMan testmempoolaccept()
887 {
888     return RPCHelpMan{"testmempoolaccept",
889-                "\nReturns result of mempool acceptance tests indicating if raw transaction (serialized, hex-encoded) would be accepted by mempool.\n"
890-                "\nThis checks if the transaction violates the consensus or policy rules.\n"
891+                "\nReturns result of mempool acceptance tests indicating if raw transaction(s) (serialized, hex-encoded) would be accepted by mempool.\n"
892+                "\nIf multiple transactions are passed in, parents must come before children and package policies apply: the transactions cannot conflict with any mempool transactions or each other.\n"
893+                "\nIf one transaction fails, other transactions may not be fully validated (the 'allowed' key will be blank).\n"

ariard commented at 4:08 am on May 27, 2021:

nit: I think a more descriptive behavior would be “if one transaction fails, remaining transactions are not submitted for validation”. See document L1146 in src/validation.cpp “Exit early to avoid doing pointless work. Update the failed tx result; the rest are unfinished”.

in src/rpc/rawtransaction.cpp:944 in 13650fe2e5

946-
947-    CMutableTransaction mtx;
948-    if (!DecodeHexTx(mtx, request.params[0].get_array()[0].get_str())) {
949-        throw JSONRPCError(RPC_DESERIALIZATION_ERROR, "TX decode failed. Make sure the tx has at least one input.");
950+    const UniValue raw_transactions = request.params[0].get_array();
951+    if (raw_transactions.size() < 1 || raw_transactions.size() > MAX_PACKAGE_COUNT) {

ariard commented at 4:45 am on May 27, 2021:

nit: the check on MAX_PACKAGE_COUNT is duplicated into AcceptMultipleTransactions. I can understand the rational of not taking the cs_main lock but I’ve a preference to keep all package policy checks in one place. That would also avoid linking packages.h in rawtransaction.cpp (assuming we hardcode the package limit in RPC doc).

glozow commented at 7:24 am on May 27, 2021:

I think of this as a distinct check, actually. We defined the testmempoolaccept API to be “maximum 25 transactions” and will return a JSONRPC error here because the user violated our API, whereas AcceptMultipleTransactions() is applying package policies.

in src/rpc/rawtransaction.cpp:976 in 13650fe2e5

1006+    }();
1007+
1008+    UniValue rpc_result(UniValue::VARR);
1009+    // We will check transaction fees we iterate through txns in order. If any transaction fee
1010+    // exceeds maxfeerate, we will keave the rest of the validation results blank, because it
1011+    // doesn't make sense to return a validation result for a transaction if its ancestor(s) would

ariard commented at 4:50 am on May 27, 2021:

nit: transaction result

in src/txmempool.h:877 in 13650fe2e5

872     const CTxMemPool& mempool;
873 
874 public:
875     CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn);
876     bool GetCoin(const COutPoint &outpoint, Coin &coin) const override;
877+    /** Add the coins created by this transaction. */

ariard commented at 5:24 am on May 27, 2021:

nit: “Temporarily add the coins created by this transaction until package processing to which it belongs is over” ?

glozow commented at 4:03 pm on May 27, 2021:

Added a more descriptive comment in #22084

in src/validation.cpp:1153 in 13650fe2e5

1154+            return PackageMempoolAcceptResult(package_state, std::move(results));
1155+        }
1156+        // Make the coins created by this transaction available for subsequent transactions in the
1157+        // package to spend. Since we already checked conflicts in the package and RBFs are
1158+        // impossible, we don't need to track the coins spent. Note that this logic will need to be
1159+        // updated if RBFs in packages are allowed in the future.

ariard commented at 5:50 am on May 27, 2021:

nit: “mempool transaction replacement by packages”.

Otherwise it lets suggest we apply RBF inside a currently-processing package. Also I think the replacement policy we’ll end up for packages is going to be far different than BIP 125, so instead of mentioning RBF, I would say simply replacement for now.

in src/validation.h:242 in 13650fe2e5

234@@ -205,6 +235,18 @@ struct MempoolAcceptResult {
235 MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx,
236                                        bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
237 
238+/**
239+* Atomically test acceptance of a package. If the package only contains one tx, package rules still apply.
240+* @param[in]    txns                Group of transactions which may be independent or contain
241+*                                   parent-child dependencies. The transactions must not conflict, i.e.
242+*                                   must not spend the same inputs, even if it would be a valid BIP125

ariard commented at 6:07 am on May 27, 2021:

A bit confusing, one might wonder if you’re aiming to in-package conflict or mempool transactions conflicted by package. I would suggest another formulation : “The package transactions must not conflict, i.e must not spend the same inputs. They must not replace mempool transaction, even if it’s valid under BIP125 rules.”

ariard commented at 6:34 am on May 27, 2021: member

ACK 13650fe

Overall code sounds robust enough for now. I did check again test coverage, both functional/unit and it seems exhaustive. I think we can still argue on few improvements around comments but not blockers IMO.

My proposal is: If a transaction in the package exceeds maxfeerate, the “reject-reason” is “max-fee-exceeded” and all subsequent transactions have blank validation results (i.e. only “txid” and “wtxid” fields). This is the same as what we do if a transaction fails validation and is still compatible with the API on master.

That’s a good-enough API for now and I agree with your proposed follow-up to calculate and evaluate the aggregated package feerate.

Other follow-ups I do have in mind :

weight units or vbytes : #20833 (review)
mempool ancestor/descendants limit for packages (#21800)
package-policy checks encapsulation : #20833 (review) (mostly motivated to have a future shared library libtxstandardness a la libbitcoinconsensus, marrying well with #21413)

What else ? I don’t think ordering really matter.

laanwj commented at 8:38 pm on May 27, 2021: member

Code review re-ACK 13650fe2e527bf0cf5d977bf5f3f1563b853ecdc My understanding is that the rest of the comments (which tend to be relating to comments, documentation, asserts, argument naming, and error messages) will be addressed in the follow-up PR, so are not blocking the merge of this feature.

laanwj merged this on May 27, 2021

laanwj closed this on May 27, 2021

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

sidhujag referenced this in commit 09fd8fccd1 on May 28, 2021

glozow deleted the branch on May 28, 2021

fanquake referenced this in commit ef8f2966ac on Jun 10, 2021

gwillen referenced this in commit 8a669eccfb on Jun 1, 2022

DrahtBot locked this on Aug 18, 2022

rpc/validation: enable packages through testmempoolaccept #20833

Conflicts