Modify testmempoolaccept to handle client’s max_raw_tx_fee check failure #21074

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

   See #21062 (review)
2. ariard added the label Feature on Feb 3, 2021
3. 
   MarcoFalke commented at 12:46 pm on February 3, 2021: member

   testmempoolaccept handles this case since it was written. See also the test

   https://github.com/bitcoin/bitcoin/blob/f72d80b07af8ecbb2f9df1e87344eb465186f19d/test/functional/rpc_rawtransaction.py#L478-L480

4. 
   MarcoFalke commented at 1:06 pm on February 3, 2021: member

   Note the reason has been changed from absurdly-high-fee to max-fee-exceeded
5. 
   ariard commented at 1:29 pm on February 3, 2021: member

   @MarcoFalke As of ea96e17, testmempoolaccept’s “allowed” field is documented “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 type of check so we should return allowed=true in that case.

   This is ambiguous and IMHO we should have a different result field to dissociate cleanly. And also nicely return fees to offending caller and such allow correction ?

6. 
   MarcoFalke commented at 4:03 pm on February 3, 2021: member

   As of ea96e17, testmempoolaccept’s “allowed” field is documented “If the mempool allows this tx to be inserted”.

   That is the case since the call was added in commit b55555da3e25a47f1e7fced7f09d4f0bf8198624.

   we should return allowed=true in that case.

   That’d be a breaking change, so I’d rather not do that.

   This is ambiguous

   This is documented, so shouldn’t be ambiguous:

   02. maxfeerate      (numeric or string, optional, default=0.10) Reject transactions whose fee rate is higher than the specified value, expressed in BTC/kB
   

   Maybe you are asking for the reject reason to be better documented in that case? Simply checking the reject reason should be sufficient in this case to dissociate cleanly.

7. 
   glozow commented at 9:04 pm on February 3, 2021: member

   Conceptually, I agree that the information given isn’t as precise as it could be. If we could start from scratch, I would do it differently. The idea for #19339 not changing allowed from False to True was, as @MarcoFalke said, because that’d actually be a breaking change, so it just changes the reject reason instead. I don’t think users would be surprised by the behavior given current documentation.

   Moving forward, if I were to propose something, I’d maybe have allowed represent “everything ok,” and then for “allowed=False” add a problem-type field for CONSENSUS, MEMPOOL_POLICY, USER_CONFIG, or UNFINISHED_VALIDATION. Just throwing the idea out there.

8. fanquake deleted a comment on Apr 10, 2021
9. laanwj closed this on May 27, 2021

   ---

10. DrahtBot locked this on Aug 18, 2022

Contributors
ariard MarcoFalke glozow

Labels
Feature