Release notes and followups from 19339 #20109

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

Xekyo 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};

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

Xekyo 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

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

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

Xekyo 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

Fabcien referenced this in commit 664bafdc08 on Nov 16, 2021

DrahtBot locked this on Feb 15, 2022