refactor: return MempoolAcceptResult from ATMP #21062

15c82d3

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

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.

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

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

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.

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.

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.

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

Tracked #21074

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.

Check now? I think it's better with enum

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 ?

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?

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.

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?

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.

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.

switched it back to push_back to adhere to my promise of "pure refactor"

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.

abc6ff1

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

Added!

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

Addressed

abc6ff1

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

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

Added comments

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

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

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?

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

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

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()?

True, done

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.

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.

ok

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

        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():

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

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

You can set m_result_type in the initializer list:

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

(Will do this 👀 )

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:

    : m_result_type{...

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

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.

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

Ok, I see this is fixed in the next commit

This should say m_base_fees

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

Agh 🤦 bad rebase.

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

same for all other members?

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

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

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)

Mm, definitely - I will keep this in mind!

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)