refactor: return MempoolAcceptResult from ATMP

glozow commented at 1:51 am on February 2, 2021: member

This is the first 4 commits of #20833, and does refactoring only. It should be relatively simple to review, and offers a few nice things:

It makes accessing values that don’t make sense (e.g. fee) when the tx is invalid an error.
Returning MempoolAcceptResult from ATMP makes the interface cleaner. The caller can get a const instead of passing in a mutable “out” param.
We don’t have to be iterating through a bunch of lists for package validation, we can just return a std::vector<MempoolAcceptResult>.
We don’t have to refactor all ATMP call sites again if/when we want to return more stuff from it.

glozow force-pushed on Feb 2, 2021

DrahtBot added the label P2P on Feb 2, 2021

DrahtBot added the label RPC/REST/ZMQ on Feb 2, 2021

DrahtBot added the label Validation on Feb 2, 2021

DrahtBot commented at 8:03 am on February 2, 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:

#21148 (Split orphan handling from net_processing into txorphanage by ajtowns)
#21055 ([Bundle 3/n] Prune remaining g_chainman usage in validation functions by dongcarl)
#21003 (test: Move MakeNoLogFileContext to libtest_util, and use it in bench by MarcoFalke)
#20750 ([Bundle 2/n] Prune g_chainman usage in mempool-related validation functions by dongcarl)
#20158 (tree-wide: De-globalize ChainstateManager by dongcarl)
#19381 (Fix UBSan warnings triggered when loading corrupt mempool.dat files by rajarshimaitra)

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/rpc/rawtransaction.cpp:975 in 15c82d3ae7 outdated

976-            if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {
977-                result_0.pushKV("reject-reason", "missing-inputs");
978-            } else {
979-                result_0.pushKV("reject-reason", strprintf("%s", state.GetRejectReason()));
980-            }
981+        if (state.IsInvalid() && state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) {

ariard commented at 12:10 pm on February 2, 2021:

15c82d3

Can you drop the state.IsInvalid() here ? I think TX_MISSING_INPUTSalways implies an invalid state (L664, src/validation.cpp).

in src/validation.cpp:1049 in abc6ff1c02 outdated

1049+    if (!ConsensusScriptChecks(args, workspace, txdata)) return MempoolAcceptResult(args.m_state);
1050 
1051     // Tx was accepted, but not added
1052-    if (args.m_test_accept) return true;
1053+    if (args.m_test_accept) {
1054+        return MempoolAcceptResult(args.m_state, std::move(args.m_replaced_transactions), args.m_fee_out);

ariard commented at 12:38 pm on February 2, 2021:

abc6ff1

What do you think about making bypass_limits part of the new MempoolAcceptResult ?

Actually we don’t have a mempool acceptance evaluation. The set of rules verified is already configurable by passing bypass_limits=true to ATMP. This flag will latch feerate and size checks (L729 and L1018 in src/validation.cpp). A consumer of this cleaner interface might be interested with the effective set of rules enforced. And consumer might not be ATMP caller who initially picked up the options.

It would be judicious if we introduce future configurable options in the future like bypass_timelocks.

glozow commented at 2:07 pm on February 2, 2021:

What do you think of changing m_accepted to an enum to encompass states beyond valid/invalid? Something like

0enum class ResultType : uint8_t {
1        UNSET, //!> Not fully validated, quit early for whatever reason.
2        INVALID, //!> Invalid.
3        VALID, //!> Valid.
4        VALID_BYPASSED, //!> some kind of limits bypassed
5}

This would leave room for bypassing timelocks as well. We probably don’t need to include more details than “valid albeit some rules were bypassed,” because the caller should already know what args they called ATMP with.

ariard commented at 11:03 am on February 3, 2021:

See my other comment but I would keep a binary state for validity. I don’t think TxValidationResult/BlockValidationResult are great examples, it makes it harder to reason on once you multiply states.

Let’s keep this suggestion in mind for now, it’s not a must for this PR. We’ll see if we need to introduce something like this if we have situation where we have one aware caller and multiple blind consumers.

in src/rpc/rawtransaction.cpp:959 in abc6ff1c02 outdated

976+    if (accept_result.m_accepted.value()) {
977+        const CAmount fee = accept_result.m_base_fees.value();
978+        // Check that fee does not exceed maximum fee
979+        if (max_raw_tx_fee && fee > max_raw_tx_fee) {
980+            result_0.pushKV("allowed", false);
981+            result_0.pushKV("reject-reason", "max-fee-exceeded");

ariard commented at 1:01 pm on February 2, 2021:

abc6ff1

I don’t know about returning “allowed”=false for max_raw_tx_fee violation. “allowed” is documented as “If the mempool allows this tx to be inserted”. Your transaction might be mempool valid but doesn’t pass the client belt-and-suspender, a different check.

Also it would be nice to return “base” and “fees” to let the transaction construction builder adapt the feerate to something passing max_raw_tx_fee.

Note, that’s independent from refactoring, more a undersight of #19339.

glozow commented at 3:11 pm on February 2, 2021:

I agree 😢 I was a bit naive when I wrote that. But it’s not part of this PR, so maybe we can discuss elsewhere

ariard commented at 11:05 am on February 3, 2021:

Tracked #21074

in src/validation.cpp:384 in abc6ff1c02 outdated

379@@ -380,10 +380,8 @@ static void UpdateMempoolForReorg(CTxMemPool& mempool, DisconnectedBlockTransact
380     auto it = disconnectpool.queuedTx.get<insertion_order>().rbegin();
381     while (it != disconnectpool.queuedTx.get<insertion_order>().rend()) {
382         // ignore validation errors in resurrected transactions
383-        TxValidationState stateDummy;
384         if (!fAddToMempool || (*it)->IsCoinBase() ||
385-            !AcceptToMemoryPool(mempool, stateDummy, *it,
386-                                nullptr /* plTxnReplaced */, true /* bypass_limits */)) {
387+            !AcceptToMemoryPool(mempool, *it, true /* bypass_limits */).m_accepted.value()) {

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

abc6ff1

I don’t think that’s a good idea to encumber code path like UpdateMempoolForReorg with std::optional. If this nullopt will throw an exception. And we do have the failure/unfinished constructor allowing m_accepted to be initialized to nullopt, even if AFAICT such constructor is never called with finished=badfor now ?

I know there is a discussion about std::optional usage here but could we restrain its usage to only m_base_fee/m_replaced_transaction ? They would be nullopt if m_accepted=false.

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

Check now? I think it’s better with enum

ariard commented at 11:20 am on February 3, 2021:

Do we really need an enum and can’t we rely only on a boolean ? Maybe you can point me to a branch how you’re using UNFINISHED because we don’t use it with this PR ?

glozow commented at 7:00 pm on February 3, 2021:

Haven’t published the branch yet but the idea is to return a std::vector<MempoolAcceptResult> from ProcessNewPackage, quit early when a tx fails, and set the not-fully-validated txns in the package to UNFINISHED. See comment?

ariard commented at 1:47 pm on February 4, 2021:

I think UNFINISHED/not_fully_validated to mark package partial failure doesn’t bring further value compared to unvalid. Do you plan to consume this UNFINISHED in a special way ? Otherwise we can just extend TxValidationResult with a PACKAGE_PARENT_FAILED instead of yet-another-state variable.

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

I think in the future with Package Relay, we may want to punish nodes differently for invalidity in packages vs invalidity in transactions, and maybe cache failed transactions differently. Obviously this isn’t set in stone, but I think it’s better to not put package-specific validation info in TxValidationState - what do you think?

ariard commented at 11:18 am on February 8, 2021:

I don’t see a straightforward reason to punish faulty pacakge-relay peers from regular tx-relay ones, at least at the mempool level. If we don’t have a motivation for UNFINISHED, let’s remove it for now and defer its introduction when we actually hit the case ? Would be easier to argue at that point.

in src/validation.cpp:1005 in abc6ff1c02 outdated

1001@@ -1007,8 +1002,7 @@ bool MemPoolAccept::Finalize(ATMPArgs& args, Workspace& ws)
1002                 hash.ToString(),
1003                 FormatMoney(nModifiedFees - nConflictingFees),
1004                 (int)entry->GetTxSize() - (int)nConflictingSize);
1005-        if (args.m_replaced_transactions)
1006-            args.m_replaced_transactions->push_back(it->GetSharedTx());
1007+        args.m_replaced_transactions.emplace_back(it->GetSharedTx());

ariard commented at 1:33 pm on February 2, 2021:

abc6ff1

Note, you might have a slight performance regression due to emplace-back memory allocation if container capacity is too small. Before this change, this potential reallocation wasn’t the default.

I think that’s okay replaced_transactions was already existent in the most busiest code path ProcessMessage.

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

switched it back to push_back to adhere to my promise of “pure refactor”

ariard commented at 11:12 am on February 3, 2021:

I think switching to push_back still do an automatic reallocation of the allocated storage if container capacity is too short. See https://www.cplusplus.com/reference/vector/vector/emplace_back/ and https://www.cplusplus.com/reference/vector/vector/push_back/.

My point was underscoring that the allocation was previously conditional of m_replaced_transaction pointer being opt-in by ATMP caller. After this change it’s not anymore. Again I don’t think that matters that most.

in src/validation.h:219 in abc6ff1c02 outdated

216+        m_accepted(true), m_state(state),
217+        m_replaced_transactions(std::move(replaced_txns)), m_base_fees(fees) {}
218+};
219+
220+/**
221+ * (Try to) add a transaction to the memory pool.

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

abc6ff1

“Evaluate a transaction mempool acceptance. If test_accept= false, add the transaction to the mempool”.

glozow commented at 3:33 pm on February 2, 2021:

Added!

ariard commented at 11:15 am on February 3, 2021:

To be precise, I think bypass_limits also overrides some feerate check (L729 in src/validation.cpp).

glozow commented at 5:34 pm on February 8, 2021:

Addressed

in src/validation.h:190 in abc6ff1c02 outdated

191+* Validation result for a single transaction mempool acceptance.
192+* When m_accepted = true, m_replaced_transactions contains a list
193+* of replaced transactions and m_base_fees contains the tx fees.
194+* When m_accepted = nullopt, the transaction was not fully validated
195+* (may be valid or invalid), e.g. when it is part of a package that
196+* failed validation for an earlier transaction.

ariard commented at 1:42 pm on February 2, 2021:

abc6ff1

See other comment about std::optional usage, but at least indicate what does m_accept = false mean.

in src/validation.h:203 in abc6ff1c02 outdated

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

ariard commented at 1:44 pm on February 2, 2021:

abc6ff1

As this interface aims to be consumed by higher users, maybe comment those variables, like “evicted transactions per BIP 125 rules”, “Raw base fees expressed in satoshis”.

glozow commented at 3:33 pm on February 2, 2021:

Added comments

ariard commented at 11:13 am on February 3, 2021:

Can you added the unit in comment (satoshi) ? I swear once you start to deal with fee computation you spend your time thinking if you’re dealing with apples or oranges :)

in src/validation.h:188 in abc6ff1c02 outdated

189-                        bool bypass_limits, bool test_accept=false, CAmount* fee_out=nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
190+/**
191+* Validation result for a single transaction mempool acceptance.
192+* When m_accepted = true, m_replaced_transactions contains a list
193+* of replaced transactions and m_base_fees contains the tx fees.
194+* When m_accepted = nullopt, the transaction was not fully validated

ariard commented at 1:51 pm on February 2, 2021:

abc6ff1

I think we should commit to a clearer terminology. Validity is always wholesome, a transaction or package is either valid or not. But validity is function of a mempool acceptance evaluation and this is configurable (bypass_limit), stateful (e.g mempool min feerate), depends if the transaction is part of a package, etc.

If we follow this line, maybe we should rename m_accepted to m_valid and have only binary state (true, false). The interface could be extended in the future to indicate it’s part of a package, and we may have a TxValidationState for package children to inherit the invalidity (PACKAGE_INVALID_PARENT).

ariard commented at 1:53 pm on February 2, 2021: member

Thanks for splitting this from #20833!

[refactor] clean up logic in testmempoolaccept

Cleans up reundant code and reduces the diff of the next commit.

9db10a5506

glozow force-pushed on Feb 2, 2021

darosior commented at 3:29 pm on February 2, 2021: member

Concept ACK -will review soon

glozow commented at 3:32 pm on February 2, 2021: member

Teeny rebase for the compiler warnings and changed from std::optional<bool> m_accepted to an enum ResultType m_result_type so there’s no risk of throwing for bad optional access.

glozow force-pushed on Feb 2, 2021

MarcoFalke removed the label P2P on Feb 4, 2021

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

MarcoFalke removed the label Validation on Feb 4, 2021

MarcoFalke added the label Refactoring on Feb 4, 2021

MarcoFalke renamed this:
~~pure refactor: return MempoolAcceptResult from ATMP~~
refactor: return MempoolAcceptResult from ATMP
on Feb 4, 2021

in src/rpc/rawtransaction.cpp:967 in bc992b7a39 outdated

984+            result_0.pushKV("vsize", virtual_size);
985+            UniValue fees(UniValue::VOBJ);
986+            fees.pushKV("base", ValueFromAmount(fee));
987+            result_0.pushKV("fees", fees);
988+        }
989+        result.push_back(std::move(result_0));

jnewbery commented at 11:11 am on February 8, 2021:

result.push_back(std::move(result_0)); is the last line in both branches of the if/else. Would it be better to leave it outside the if/else?

jnewbery commented at 5:29 pm on February 8, 2021:

I guess this is because it’s better to structure like this for package acceptance.

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

It makes more sense with the package all valid / 1 invalid tx branches, but not as much here. I suppose leaving it like this reduces the diff for later

in src/validation.cpp:689 in bc992b7a39 outdated

685@@ -688,10 +686,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
686         return false; // state filled in by CheckTxInputs
687     }
688 
689-    // If fee_out is passed, return the fee to the caller
690-    if (args.m_fee_out) {
691-        *args.m_fee_out = nFees;
692-    }
693+    ws.m_base_fees = nFees;

jnewbery commented at 11:29 am on February 8, 2021:

Do we even need the local nFees variable, now that ws.m_base_fees is always set? Can you just pass a reference to ws.m_base_fees to CheckInputs()?

glozow commented at 3:26 pm on February 8, 2021:

True, done

in src/validation.h:201 in bc992b7a39 outdated

202+    ResultType m_result_type;
203+    TxValidationState m_state;
204+
205+    // The following fields are only present when m_result_type = ResultType::VALID
206+    /** Mempool transactions replaced by the tx per BIP 125 rules. */
207+    std::optional<std::list<CTransactionRef>> m_replaced_transactions;

jnewbery commented at 11:32 am on February 8, 2021:

I don’t think the std::optional wrapper is adding anything here. Essentially all it does is add a boolean where false means the value is undefined and true means that it’s defined. You’re already saying that these fields are only defined if m_result_type = ResultType::VALID, so unless you think there’s some reason to add this for safety or clarity, I wouldn’t bother.

glozow commented at 2:44 pm on February 8, 2021:

I think there should be a distinction between “tx didn’t replace any txns” and “tx was invalid and thus couldn’t replace any txns,” and it should be an error to try to look at m_replaced_transactions if the tx was invalid.

jnewbery commented at 5:29 pm on February 8, 2021:

ok

in src/validation.h:192 in bc992b7a39 outdated

193+struct MempoolAcceptResult {
194+    /** Used to indicate the results of mempool validation,
195+    * including the possibility of unfinished validation.
196+    */
197+    enum class ResultType : uint8_t {
198+        UNFINISHED, //!> Not fully validated.

jnewbery commented at 11:40 am on February 8, 2021:

The tristate here could be confusing for reviewers without the context of package acceptance. Perhaps this comment could be expanded a little. Something like:

0        UNFINISHED, //!> Not fully validated. Only used for package mempool acceptance where an individual tx may not be fully validated if the package fails.

Also consider adding an out-assertion to AcceptToMemoryPool():

0MempoolAcceptResult AcceptToMemoryPool(CTxMemPool& pool, const CTransactionRef &tx, bool bypass_limits, bool test_accept)
1{
2    auto res = AcceptToMemoryPoolWithTime(Params(), pool, tx, GetTime(), bypass_limits, test_accept);
3    // For single tx acceptance, validation must not be partial.
4    assert(res.m_result_type == MempoolAcceptResult::ResultType::VALID ||
5           res.m_result_type == MempoolAcceptResult::ResultType::INVALID);  
6    return res;

glozow commented at 3:26 pm on February 8, 2021:

Resolved by removing UNFINISHED, I will plan to add this assertion and docs whenever we have more than 2 result types 👍

in src/validation.h:208 in bc992b7a39 outdated

209+    std::optional<CAmount> m_base_fees;
210+
211+    /** Constructor for failure or unfinished case */
212+    explicit MempoolAcceptResult(TxValidationState state, bool finished=true) :
213+        m_state(state), m_replaced_transactions(nullopt), m_base_fees(nullopt) {
214+            m_result_type = finished ? ResultType::INVALID : ResultType::UNFINISHED;

jnewbery commented at 11:53 am on February 8, 2021:

You can set m_result_type in the initializer list:

0        m_result_type(finished ? ResultType::INVALID : ResultType::UNFINISHED),
1        m_state(state), m_replaced_transactions(nullopt), m_base_fees(nullopt) {}

glozow commented at 3:26 pm on February 8, 2021:

(Will do this 👀 )

jnewbery commented at 11:55 am on February 8, 2021: member

Code review ACK bc992b7a394629137929647998149f18fea5ab29

A few non-essential style suggestions inline. You’re going to be touching this code again in the next PRs, so feel free to defer these suggestions.

glozow force-pushed on Feb 8, 2021

glozow commented at 3:24 pm on February 8, 2021: member

Addressed @jnewbery comments and @ariard #21062 (review) and removed the ResultType::UNFINISHED for now since it’s unused. Leaving it as an enum because I think we agree that there’s value in having more than 2 states, and the struct is the same size

jnewbery commented at 5:32 pm on February 8, 2021: member

Code review ACK a9ff9c1ca0530e448341e9d24ecd5f8bc6f2ee42

Cirrus failure looks spurious, but I don’t know how to restart it.

in src/validation.h:212 in 1407daff4a outdated

213+        m_state(state), m_replaced_transactions(nullopt), m_base_fees(nullopt) {}
214+
215+    /** Constructor for success case */
216+    explicit MempoolAcceptResult(TxValidationState state,
217+                                 std::list<CTransactionRef>&& replaced_txns, CAmount fees) :
218+        m_result_type(ResultType::VALID), m_state(state),

MarcoFalke commented at 9:25 am on February 9, 2021:

What is the point of setting the state when the default constructed one is already valid? It seems like a footgun to allow the caller to pass an invalid state here. At least, it should ASSUME(state.IsValid());.

Also, clang-format will like you more when you put the : in the next line:

0    : m_result_type{...

glozow commented at 2:56 pm on February 9, 2021:

You’re right, there’s no point in passing the state when it’s successful

in src/validation.h:191 in 1407daff4a outdated

192+*/
193+struct MempoolAcceptResult {
194+    /** Used to indicate the results of mempool validation,
195+    * including the possibility of unfinished validation.
196+    */
197+    enum class ResultType : uint8_t {

MarcoFalke commented at 9:27 am on February 9, 2021:

nit: If ResultType is never serialized or used as an enum flag, you don’t need to specify the underlying type. And I think the compiler will fill this to at least 32 bits inside the struct anyway, so it won’t give you any space savings either.

in src/validation.cpp:1069 in 1407daff4a outdated

1077+    TxValidationState state;
1078     std::vector<COutPoint> coins_to_uncache;
1079-    MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, plTxnReplaced, bypass_limits, coins_to_uncache, test_accept, fee_out };
1080-    bool res = MemPoolAccept(pool).AcceptSingleTransaction(tx, args);
1081-    if (!res) {
1082+    MemPoolAccept::ATMPArgs args { chainparams, state, nAcceptTime, {}, bypass_limits, coins_to_uncache, test_accept, {} };

MarcoFalke commented at 9:28 am on February 9, 2021:

The two {}, as well as state aren’t args, but workspace variables. Is there any reason to not put them in the workspace?

MarcoFalke commented at 9:38 am on February 9, 2021:

Ok, I see this is fixed in the next commit

in src/validation.cpp:492 in 64fb766938 outdated

485@@ -489,14 +486,17 @@ class MemPoolAccept
486         CTxMemPool::setEntries m_all_conflicting;
487         CTxMemPool::setEntries m_ancestors;
488         std::unique_ptr<CTxMemPoolEntry> m_entry;
489+        std::list<CTransactionRef> m_replaced_transactions;
490 
491         bool m_replacement_transaction;
492+        CAmount m_fee_out;

MarcoFalke commented at 9:36 am on February 9, 2021:

This should say m_base_fees

0validation.cpp:684:103: error: no member named 'm_base_fees' in '(anonymous namespace)::MemPoolAccept::Workspace'
1    if (!Consensus::CheckTxInputs(tx, state, m_view, g_chainman.m_blockman.GetSpendHeight(m_view), ws.m_base_fees)) {
2                                                                                                   ~~ ^
3validation.cpp:702:24: error: no member named 'm_base_fees' in '(anonymous namespace)::MemPoolAccept::Workspace'
4    nModifiedFees = ws.m_base_fees;
5                    ~~ ^
6validation.cpp:716:45: error: no member named 'm_base_fees' in '(anonymous namespace)::MemPoolAccept::Workspace'
7    entry.reset(new CTxMemPoolEntry(ptx, ws.m_base_fees, nAcceptTime, ::ChainActive().Height(),
8                                         ~~ ^
93 errors generated.

glozow commented at 3:06 pm on February 9, 2021:

Agh 🤦 bad rebase.

MarcoFalke dismissed

MarcoFalke commented at 9:37 am on February 9, 2021: member

The third commit doesn’t compile

e2713614089f77a01c98bb6783f2b1a22aa6ab13 🎪

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3e2713614089f77a01c98bb6783f2b1a22aa6ab13 🎪
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUidXwwAiGoRXoyy71GeKNoMewKKtoqWSYvjSb9iQMeQIIsS9u0CZA6zPQxjkE9J
 86uynw8usu+HcHAKQ852r4hmrmgJZiuZalgGPVAfJbODgp89rJzhbVElwiwW77xHh
 9EYUET22yB00viK1zkIzIMtcPsNhcyTfxTYZIxI0DP/bqFBkpwAmTP3axwqmaoLAN
10iHtInc+D4ajOTGWwzimQShQBvjPjeFvtf/ErqaS7Nj1IqGCXLu9nadt14Gf0OcgS
11MTceMpLpPZVGGHJedlWtXrLFICSGFTNWsZQ7GcDQEPX8axNligCDtIZZ/PHhi5j9
12Z52tD04emkuxI+mma80ejcnkKh4oI88dG9/GL4C8Qhua7MCJHzyASY7kmHZ7hqJB
13Og8AYIXJVFHXw0wsgiIdhDQ3A4bwX31T8xc5lhpvSaZUJgnbih4QdA6oohHr4vRf
14Z+Sx0DgksuscH8LIVgcJwcCLukX2Wb5IRvBiR3cSclQM20WEL66zt8ce15nMVECa
15zO++FfcZ
16=jSjm
17-----END PGP SIGNATURE-----

Timestamp of file with hash 1a5436e3da8426ac64d2e23be49dd4c3bbb869d0d35ecba05ef4c37aaa88e2a9 -

[refactor] return MempoolAcceptResult

This creates a cleaner interface with ATMP, allows us to make results const,
and makes accessing values that don't make sense (e.g. fee when tx is
invalid) an error.

f82baf0762

[refactor] const ATMPArgs and non-const Workspace

ATMPArgs should contain const arguments for validation.
The Workspace should contain state that may change
throughout validation.

174cb5330a

[refactor] improve style for touched code 53e716ea11

glozow force-pushed on Feb 9, 2021

glozow commented at 3:09 pm on February 9, 2021: member

The third commit doesn’t compile

Should be fixed now 🤦 thanks @MarcoFalke! Fixed the constructors a bit as well to take out the unnecessary TxValidationState in the success case and Assume not valid in the failure case.

MarcoFalke commented at 11:37 am on February 10, 2021: member

Changes since previous review:

No longer pass redundant default-constructed state when valid
Remove uint8_t from enum class
Add Assume(!state.IsValid());
Make it compile

ACK 53e716ea119658c28935fee24eb50090907c500e 💿

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 53e716ea119658c28935fee24eb50090907c500e 💿
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh02Qv/VKTbpbQkoToYPmzKcSX5aJ+JAaQThI2/aGnpdgFnl/GdIVDyMcWHyygP
 8iduWhmF6BeEerKuctDAbfLB+EjeQdIr0xMa6lg5nkSsZTi0W+wn+5okOnngsjAzI
 9JazlKJJGQj/WjUJof04cTtIfcT9ecLS9yN8TJ/W8EWnm+RpSCOMBgUvRfBgdCBXX
10GKs2xoxN6BT1ML5+y7qQQB2VyZE/DAEykbR6NhgaAIkrA5UZM6tP4t5/YTNtjoZq
11hBgKkesPqym/WjB/6LTEwannfiW/ACvpM3Czg5pPS1svMyMxT4LQh4rMDiF4U21x
12apPhPIMwvGce1pdX6UwMuDhRdA0VE6y23rxjSjhZaA0ZEBOVEW2sYBCa9bVcROrY
1360WzJ2j6dYY2xIHJsYo5vG1LxjSrz42My50LLIKTBsihTg1LNst52KXZfFjozng6
142gewYrzP0lJe/fyUf9BxNK1w9+mbd5kCJDR1VHxsu9Yc5B+4gVKmva4uFodLd+Yo
15Hyw2/p/q
16=G4nn
17-----END PGP SIGNATURE-----

Timestamp of file with hash 5da03708a5e913024d3ccd093bac9c7a16f54f296f91a24b4c60d85316f124f4 -

in src/validation.h:196 in 53e716ea11

197+    */
198+    enum class ResultType {
199+        VALID, //!> Fully validated, valid.
200+        INVALID, //!> Invalid.
201+    };
202+    ResultType m_result_type;

MarcoFalke commented at 11:39 am on February 10, 2021:

nit: Would be nice if this was const, so that compilation fails instead of getting a valgrind error on runtime if this is unset.

in src/validation.h:197 in 53e716ea11

198+    enum class ResultType {
199+        VALID, //!> Fully validated, valid.
200+        INVALID, //!> Invalid.
201+    };
202+    ResultType m_result_type;
203+    TxValidationState m_state;

MarcoFalke commented at 11:39 am on February 10, 2021:

same for all other members?

in src/validation.h:214 in 53e716ea11

215+            Assume(!state.IsValid()); // Can be invalid or error
216+        }
217+
218+    /** Constructor for success case */
219+    explicit MempoolAcceptResult(std::list<CTransactionRef>&& replaced_txns, CAmount fees)
220+        : m_result_type(ResultType::VALID), m_state(TxValidationState{}),

MarcoFalke commented at 11:40 am on February 10, 2021:

Haven’t tried, but I think this can be written shorter

0        : m_result_type(ResultType::VALID), m_state{},

in src/validation.cpp:564 in 174cb5330a outdated

560     const int64_t nAcceptTime = args.m_accept_time;
561     const bool bypass_limits = args.m_bypass_limits;
562     std::vector<COutPoint>& coins_to_uncache = args.m_coins_to_uncache;
563 
564     // Alias what we need out of ws
565+    TxValidationState &state = ws.m_state;

MarcoFalke commented at 11:42 am on February 10, 2021:

nit in 174cb5330af4b09f3a66974d3bae783ea43b190e: When touching the code it would be good if the correct style was used right away, instead of adding another commit afterward to fix it up once more. (This increases the git blame and git log -S depth)

glozow commented at 6:26 pm on February 10, 2021:

Mm, definitely - I will keep this in mind!

in src/validation.cpp:702 in 174cb5330a outdated

698@@ -702,7 +699,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
699     int64_t nSigOpsCost = GetTransactionSigOpCost(tx, m_view, STANDARD_SCRIPT_VERIFY_FLAGS);
700 
701     // nModifiedFees includes any fee deltas from PrioritiseTransaction
702-    nModifiedFees = nFees;
703+    nModifiedFees = ws.m_fee_out;

MarcoFalke commented at 11:44 am on February 10, 2021:

nit 174cb5330af4b09f3a66974d3bae783ea43b190e: When touching a member variable in all places where it is used it could make sense to use the right name right away instead of using the wrong name first in all sites and then adding a commit to rename it. (Increases git blame depth)

MarcoFalke commented at 11:44 am on February 10, 2021: member

Left some non-critical style nits

ariard commented at 12:10 pm on February 10, 2021: member

Code Review ACK 53e716e, I did tweak a bit the touched paths to see if we had good test coverage. Didn’t find holes.

Nit: if you have to re-touch the code, you forget this one #21062 (review)

jnewbery commented at 6:39 pm on February 10, 2021: member

Code review ACK 53e716ea119658c28935fee24eb50090907c500e

glozow force-pushed on Feb 10, 2021

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

Opened followup #21146 to address style comments from @MarcoFalke and @ariard (oopsie for accidentally pushing here, please ignore that).

MarcoFalke merged this on Feb 11, 2021

MarcoFalke closed this on Feb 11, 2021

glozow deleted the branch on Feb 11, 2021

sidhujag referenced this in commit 42d3336c78 on Feb 11, 2021

MarcoFalke referenced this in commit edf679503c on Apr 28, 2021

sidhujag referenced this in commit dd929c7eea on Apr 28, 2021

Fabcien referenced this in commit 9c234285e9 on Jul 22, 2022

Fabcien referenced this in commit 736e023876 on Jul 22, 2022

Fabcien referenced this in commit 9aa6dcdcc3 on Jul 22, 2022

Fabcien referenced this in commit fc122b7446 on Jul 22, 2022

DrahtBot locked this on Aug 18, 2022

refactor: return MempoolAcceptResult from ATMP #21062

Conflicts