I would rather keep coin cache cleanup at this location. After this change, it's spread in two locations : in case of success evaluation in AcceptSingleTransaction, in case of failure in MemPoolAccept destructor. A reviewer has to read both locations now.

Further, it's harder to qualify that cleanup is reserved for success path in AcceptSingleTransaction, considering that MemPoolAcceptResult internal state is provided by Workspace.

Also, I don't know about relying on RAII semantic, the destructor will be only called when object is going out of scope. In that case, does it mean we'll only cleanup when caller ends up ? For e.g, if the caller was BroadcastTransaction, may the coin cache cleanup on cs_main lock tacking before RelayTransaction ?

After this change, it's spread in two locations : in case of success evaluation in AcceptSingleTransaction, in case of failure in MemPoolAccept destructor.

Not really. The only location a reviewer has to check is wherever they want to keep the coins in cache. Also, the change here means that if someone forgets to consider uncaching, it defaults to uncache all. This is safer.

Also, I don't know about relying on RAII semantic, the destructor will be only called when object is going out of scope. In that case, does it mean we'll only cleanup when caller ends up ? For e.g, if the caller was BroadcastTransaction, may the coin cache cleanup on cs_main lock tacking before RelayTransaction ?

The two possible callers of MemPoolAccept are ATMP and ProcessNewPackage They invoke it like this:

const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args);

afaik it goes out of scope immediately, before the lock is released.

Not really. The only location a reviewer has to check is wherever they want to keep the coins in cache.

Depends what you're willingly to convince yourself by reviewing. Assuming uncaching in case of failures is the correct behavior, looking at the proposed changes, you have to a) verify that uncaching code is well-implemented in MemPoolAccept destructor, b) identify that caching conservation is covering all success paths and thus dissociate success code path from errors one in AcceptSingleTransaction and c) ensure the RAAI semantic is enforced as intended. IMHO, more complex than current one-place location.

That said, I think the code is correct, and each one review process and code style flavors might turn as really abstract discussion so not going to argue further :)

The two possible callers of MemPoolAccept are ATMP and ProcessNewPackage They invoke it like this:

const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args);

afaik it goes out of scope immediately, before the lock is released.

I believe you're right. In that expression, MemPoolAccept() is a temporary, and the lifetime rules for temporaries are

All temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created, and if multiple temporary objects were created, they are destroyed in the order opposite to the order of creation. This is true even if that evaluation ends in throwing an exception.

https://en.cppreference.com/w/cpp/language/lifetime#Temporary_object_lifetime

Note that there are two exceptions to that rule, neither of which apply here:

• The lifetime of a temporary object may be extended by binding to a const lvalue reference or to an rvalue reference (since C++11), see reference initialization for details.
• The lifetime of a temporary object created when evaluating the default arguments of a default constructor used to initialize an element of an array ends before the next element of the array begins initialization.