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

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

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