Release notes and followups from 19339 #20109

pull glozow wants to merge 2 commits into bitcoin:master from glozow:docs-absurdfee changing 2 files +16 −5

glozow commented at 9:25 PM on October 8, 2020: member

Pretty trivial... addresses some tiny comments from #19339. Also fixes a docs typo from #19940 and adds a release note about the error message change for testmempoolaccept.
style and nits for fee-checking in BroadcastTransaction c201d73df3
DrahtBot added the label Docs on Oct 8, 2020
glozow commented at 12:25 AM on October 9, 2020: member

@MarcoFalke @instagibbs :)
MarcoFalke added this to the milestone 0.21.0 on Oct 9, 2020
MarcoFalke commented at 7:11 AM on October 9, 2020: member

Note that sendrawtransaction changed the return code and error string

in src/node/transaction.cpp:17 in c201d73df3 outdated

  12 | @@ -13,7 +13,8 @@
  13 |  
  14 |  #include <future>
  15 |  
  16 | -static TransactionError HandleATMPError(const TxValidationState& state, std::string& err_string_out) {
  17 | +static TransactionError HandleATMPError(const TxValidationState& state, std::string& err_string_out)
  18 | +{

murchandamus commented at 10:20 AM on October 9, 2020:

I see that this follows the rule for braces from the coding style putting braces on new lines for methods: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

in src/node/transaction.cpp:57 in c201d73df3 outdated

  50 | @@ -50,10 +51,10 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
  51 |      if (!node.mempool->exists(hashTx)) {
  52 |          // Transaction is not already in the mempool.
  53 |          TxValidationState state;
  54 | -        CAmount fee{0};
  55 | -        if (max_tx_fee) {
  56 | +        if (max_tx_fee > 0) {
  57 |              // First, call ATMP with test_accept and check the fee. If ATMP
  58 |              // fails here, return error immediately.
  59 | +            CAmount fee{0};

murchandamus commented at 10:23 AM on October 9, 2020:

I noticed that the declaration of fee here was moved to a closer scope. As fee does not get used outside of the scope this appears to not change behaviour.

glozow commented at 3:04 PM on October 9, 2020:

Yep, just reducing the scope :)

in doc/release-notes.md:108 in e5865f08a5 outdated
```
 101 | @@ -102,9 +102,12 @@ will trigger BIP 125 (replace-by-fee) opt-in. (#11413)
 102 |    option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
 103 |    removed in the next major release. (#19469)
 104 |  
 105 | -- The `testmempoolaccept` RPC returns `vsize` and a `fee` object with the `base` fee
 106 | +- The `testmempoolaccept` RPC returns `vsize` and a `fees` object with the `base` fee
 107 |    if the transaction passes validation. (#19940)
 108 |  
 109 | +- The `testmempoolaccept` RPC returns `max-fee-exceeded` rather than `absurdly-high-fee`
```
murchandamus commented at 10:37 AM on October 9, 2020:

Given that the criteria to throw this reject reason appears to be the exceeding of a feerate, would it not be more informative to make the reject-reason max-feerate-exceeded instead of max-fee-exceeded? I realize that this would have been a better feedback for #19339, but I only see it now.

glozow commented at 4:02 PM on October 9, 2020:

Thanks for reviewing :) my "inspiration" was TransactionError::MAX_FEE_EXCEEDED, hopefully not too big of a deal

murchandamus commented at 4:09 PM on October 9, 2020:

It's a bit of a pet-peeve of mine that fees and feerates are often mixed up in Bitcoin, but the ship has probably sailed for this change? :thinking: I'll have to be faster next time. ;)

sipa commented at 5:20 PM on October 9, 2020:

@Xekyo You're absolutely right that they're often mixed up, but in this context it is actually referring to an absolute fee (by default 0.1 BTC...) rather than a feerate.

murchandamus commented at 5:33 PM on October 9, 2020:

Okay, thanks for the clarification. I'm a bit confused by this description then:

The sendrawtransaction error code for exceeding maxfeerate has been changed from -26 to -25. The error string has been changed from "absurdly-high-fee" to "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)." The testmempoolaccept RPC returns max-fee-exceeded rather than absurdly-high-fee as the reject-reason. (#19339)

It sounds to me that exceeding maxfeerate causes the reject-reason max-fee-exceeded. If they are referring to two separate error resolutions, it wasn't apparent to me. I'm not overtly invested, though, if that looks fine to more experienced contributors.

sipa commented at 5:35 PM on October 9, 2020:

Oh perhaps it is me who is confusing things.

glozow commented at 11:40 PM on October 9, 2020:

The maxfeerate passed into testmempoolaccept is a feerate and the wallet -maxtxfee option is a fee amount. I'm not really sure why this is the case 😅
glozow force-pushed on Oct 9, 2020
glozow commented at 3:07 PM on October 9, 2020: member

Sorry @MarcoFalke I confused myself 😅 , added notes for the changed error codes and strings.

in doc/release-notes.md:105 in 8f214774e2 outdated

 101 | @@ -102,8 +102,15 @@ will trigger BIP 125 (replace-by-fee) opt-in. (#11413)
 102 |    option `-deprecatedrpc=banscore` is used. The `banscore` field will be fully
 103 |    removed in the next major release. (#19469)
 104 |  
 105 | -- The `testmempoolaccept` RPC returns `vsize` and a `fee` object with the `base` fee
 106 | -  if the transaction passes validation. (#19940)
 107 | +- User-configured maximum feerates are no longer considered network rules. The

MarcoFalke commented at 3:32 PM on October 9, 2020:

The never have been? The code happened to be in ATMP, but never executed for network txs.

- The

glozow commented at 3:59 PM on October 9, 2020:

I guess I was trying to justify the error code change but yeah it's irrelevant here.

in doc/release-notes.md:110 in 8f214774e2 outdated

 107 | +- User-configured maximum feerates are no longer considered network rules. The
 108 | +  `sendrawtransaction` error code for exceeding `maxfeerate` has been changed from
 109 | +  `-26` to `-25`. The error string has been changed from "absurdly-high-fee" to
 110 | +  "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)." The
 111 | +  `testmempoolaccept` RPC returns `max-fee-exceeded` rather than `absurdly-high-fee`
 112 | +  as the `reject-reason` if the transaction's feerate exceeds `maxfeerate`. (#19339)

MarcoFalke commented at 3:34 PM on October 9, 2020:

I think this section can be moved to "low level changes" --> RPC section

glozow commented at 3:59 PM on October 9, 2020:

Done

MarcoFalke approved
MarcoFalke commented at 3:34 PM on October 9, 2020: member

ACK
[doc] release notes for max fee checking 88197b0769
glozow force-pushed on Oct 9, 2020
MarcoFalke commented at 4:05 PM on October 9, 2020: member

cr re-ACK 88197b0769770913941a3361bff3a1c67a86f7d2

in doc/release-notes.md:358 in 88197b0769

 353 | +  `testmempoolaccept` RPC returns `max-fee-exceeded` rather than `absurdly-high-fee`
 354 | +  as the `reject-reason`. (#19339)
 355 | +
 356 | +- To make wallet and rawtransaction RPCs more consistent, the error message for
 357 | +  exceeding maximum feerate has been changed to "Fee exceeds maximum configured by user
 358 | +  (e.g. -maxtxfee, maxfeerate)." (#19339)

murchandamus commented at 4:11 PM on October 9, 2020:

Great improvement! :+1:

RiccardoMasutti approved
RiccardoMasutti commented at 7:48 PM on October 12, 2020: contributor

ACK

in doc/release-notes.md:351 in 88197b0769

 346 | @@ -347,6 +347,16 @@ RPC
 347 |    - Fee estimation failed
 348 |    - Transaction has too long of a mempool chain
 349 |  
 350 | +- The `sendrawtransaction` error code for exceeding `maxfeerate` has been changed from
 351 | +  `-26` to `-25`. The error string has been changed from "absurdly-high-fee" to

achow101 commented at 6:28 PM on October 13, 2020:

I think this should also list the name for these error codes as the number itself is a harder to undertsand.

fanquake commented at 4:19 AM on October 14, 2020:

We can adjust this in the wiki closer to release.

achow101 commented at 6:29 PM on October 13, 2020: member

ACK 88197b0769770913941a3361bff3a1c67a86f7d2
fanquake merged this on Oct 14, 2020
fanquake closed this on Oct 14, 2020
sidhujag referenced this in commit 6d323e260d on Oct 15, 2020
adamjonas cross-referenced this on Jan 6, 2021 from issue Followup with potential downstream breakage after #19339 by fanquake
Fabcien referenced this in commit 664bafdc08 on Nov 16, 2021
bitcoin locked this on Feb 15, 2022

Contributors

Labels

Milestone
0.21.0

Linked (view graph)

#19339 validation: re-delegate absurd fee checking from mempool to clients #19940 rpc: Return fee and vsize from testmempoolaccept #20099 Followup with potential downstream breakage after #19339