rpc: Use the default maxfeerate value as BTC/kB #16521

remagpie · 2019-08-01T17:43:43Z

This patch tries to treat maxfeerate in sendrawtransaction/testmempoolaccept RPC as a rate(BTC/kB) instead of an absolute value(BTC).
The included test case checks if the new behavior works correctly, by using the transaction with an absolute fee of ~0.02BTC, where the fee rate is ~0.2BTC/kB.
This test should be failing if the default maxfeerate is 0.1BTC, but pass if the default value is 0.1BTC/kB

maflcko

Thanks for working on this. A few questions.

src/rpc/rawtransaction.cpp

test/functional/rpc_rawtransaction.py

DrahtBot · 2019-08-01T19:48:42Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

maflcko · 2019-08-02T13:29:55Z

A few tests need to be updated, since the default is no longer an absolute value of 0.01, but a fee rate of 0.01 BTC/kB. Generally txs are less than 1kB, so the absurd high-fee will kick in earlier.

promag · 2019-08-02T14:04:59Z

Concept ACK.

AcceptToMemoryPool could be changed to receive absurd_fee_rate instead? Then computing the transaction fee could be moved there (reducing duplicate code) and DEFAULT_MAX_FEE_RATE could be moved to validation.h.

Should backport this?

maflcko · 2019-08-02T15:25:08Z

@promag The fee check has nothing to do with validation, so the default value should not live in validation. See also " [WIP] Remove nAbsurdFee fee from AcceptToMemoryPool #15810 " by @jnewbery

maflcko · 2019-08-02T15:27:23Z

Also, I'd rather not change validation in a bugfix for an rpc.

promag · 2019-08-02T15:31:41Z

@MarcoFalke sorry for not being clear on my comment, I wasn't suggesting to do that change here, hence the backport question.

Thanks for referencing #15810.

maflcko · 2019-08-02T15:34:27Z

This was introduced here: #16382 (comment), so no it does not need backport.

maflcko

Looks good. Some style comments

src/rpc/rawtransaction.cpp

test/functional/feature_segwit.py

test/functional/wallet_basic.py

maflcko · 2019-08-05T19:28:38Z

ACK 7335bd3

remagpie · 2019-08-06T02:37:01Z

One of the Travis jobs has failed by timeout.
I have no idea why it failed though.
Should I investigate about it?

Sjors · 2019-08-06T12:26:06Z

I restarted Travis for you.

ACK 7335bd3, though I suggest having the RPC code catch absurdly-high-fee from validation and change it to absurdly-high-fee-rate.

The rename from DEFAULT_MAX_RAW_TX_FEE to DEFAULT_MAX_RAW_TX_FEE_RATE adds a much needed clarification. It's less easy to confuse with DEFAULT_TRANSACTION_MAXFEE (-maxtxfee).

We're still in a bit of weird situation, but it's fine for now: fundrawtransaction has a feeRate arguments and checks absolute fee against -maxtxfee, while sendrawtransaction checks the feeRate (against maxfeerate), and doesn't check the absolute fee.

maflcko · 2019-08-06T14:21:33Z

cc @kallewoof

kallewoof · 2019-08-07T08:33:14Z

test/functional/rpc_rawtransaction.py

        rawTx = self.nodes[2].createrawtransaction(inputs, outputs)
        rawTxSigned = self.nodes[2].signrawtransactionwithwallet(rawTx)
        assert_equal(rawTxSigned['complete'], True)
-        # 1000 sat fee, ~100 b transaction, fee rate should land around 10 sat/b = 0.00010000 BTC/kB
+        # 2000000 sat fee, ~100 b transaction, fee rate should land around 20000 sat/b = 0.20000000 BTC/kB


This tests the default, but removes the test for when a user provides a max fee rate below the default. That is probably the more frequent use case, so it should be tested.

Agreed. I'll refactor the test a little bit so that both cases are covered.

remagpie · 2019-08-07T13:56:14Z

@Sjors I agree with the rationale behind your suggestion, but I can't think of a neat way of actually implementing it.
I prefer leaving it as is, but I'm open to suggestion if there's any good idea for the implementation.

maflcko · 2019-08-12T18:47:47Z

ACK 49b1f48

Show signature and timestamp

Signature:

Timestamp of file with hash 9a3ed53c1fee1a38984565afc1e5e4de52ffa6dde5c6b84909b5d9645e01d6d1 -

practicalswift · 2019-08-18T11:02:39Z

src/rpc/rawtransaction.cpp

 */
-constexpr static CAmount DEFAULT_MAX_RAW_TX_FEE{COIN / 10};
+const static CFeeRate DEFAULT_MAX_RAW_TX_FEE_RATE{COIN / 10};


Style nit: The storage-class specifier is typically put at the start of the declaration in this project -- that is: static const … is typically used instead of const static … :-)

This patch adds test for the following case - maxfeerate is omitted, and the actual fee rate is higher than the default value(0.1 BTC/kB)

maflcko · 2019-09-16T19:17:06Z

re-run ci

remagpie · 2019-09-17T02:08:38Z

I couldn't reproduce the failing test on my machine. Is it CI's problem?

laanwj · 2019-09-18T14:47:38Z

ACK 2dfd683 (ACKs by Sjors and MarcoFalke above for trivially different code)

2dfd683 test: Add test for default maxfeerate in sendrawtransaction (Joonmo Yang) 261843e wallet/rpc: Use the default maxfeerate value as BTC/kB (Joonmo Yang) Pull request description: Fixes #16382 This patch tries to treat `maxfeerate` in sendrawtransaction/testmempoolaccept RPC as a rate(BTC/kB) instead of an absolute value(BTC). The included test case checks if the new behavior works correctly, by using the transaction with an absolute fee of ~0.02BTC, where the fee rate is ~0.2BTC/kB. This test should be failing if the default `maxfeerate` is 0.1BTC, but pass if the default value is 0.1BTC/kB ACKs for top commit: laanwj: ACK 2dfd683 (ACKs by Sjors and MarcoFalke above for trivially different code) Tree-SHA512: a1795bffe8a182acef8844797955db1f60bb0c0ded97148f3572dc265234d5219271a3a7aa0b6418a43f73b2b2720ef7412ba169c99bb1cdcac52051f537d6af

maflcko · 2019-09-18T18:36:11Z

post-merge re-ACK 2dfd683 (only change is style nit)

Show signature and timestamp

Signature:

Timestamp of file with hash fbaf9ebc856aec97f9850a70c2b6a2435c1b4ab700db0b49752d9eb74c89975e -

maflcko · 2019-09-18T19:00:52Z

test/functional/rpc_rawtransaction.py

        txId = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 1.0)
        rawTx = self.nodes[0].getrawtransaction(txId, True)
        vout = next(o for o in rawTx['vout'] if o['value'] == Decimal('1.00000000'))

        self.sync_all()
        inputs = [{ "txid" : txId, "vout" : vout['n'] }]
-        outputs = { self.nodes[0].getnewaddress() : Decimal("0.99999000") } # 1000 sat fee
+        outputs = { self.nodes[0].getnewaddress() : Decimal("0.999990000") } # 10000 sat fee


eh, actually this is incorrect. Appending zeros after the number after the decimal point does not change the number.

>>> 0.9 == 0.90 True

Yeah, should be 0.99990000

OMG. I'll send a patch soon.

Yeah, should be 0.99990000

Done in #16929.

2dfd683 test: Add test for default maxfeerate in sendrawtransaction (Joonmo Yang) 261843e wallet/rpc: Use the default maxfeerate value as BTC/kB (Joonmo Yang) Pull request description: Fixes bitcoin#16382 This patch tries to treat `maxfeerate` in sendrawtransaction/testmempoolaccept RPC as a rate(BTC/kB) instead of an absolute value(BTC). The included test case checks if the new behavior works correctly, by using the transaction with an absolute fee of ~0.02BTC, where the fee rate is ~0.2BTC/kB. This test should be failing if the default `maxfeerate` is 0.1BTC, but pass if the default value is 0.1BTC/kB ACKs for top commit: laanwj: ACK 2dfd683 (ACKs by Sjors and MarcoFalke above for trivially different code) Tree-SHA512: a1795bffe8a182acef8844797955db1f60bb0c0ded97148f3572dc265234d5219271a3a7aa0b6418a43f73b2b2720ef7412ba169c99bb1cdcac52051f537d6af

6659810 test: use named args for sendrawtransaction calls (Jon Atack) 5c1cd78 doc: improve rawtransaction code/test docs (Jon Atack) acc14c5 test: fix incorrect value in rpc_rawtransaction.py (Jon Atack) Pull request description: Follow-up to PR #16521. - Fix incorrect value in rpc_rawtransaction test as per https://github.com/bitcoin/bitcoin/pull/16521/files#r325842308 - Improve the code docs - Use named arguments as per https://github.com/bitcoin/bitcoin/pull/16521/files#r310715127 Happy to squash or keep only the first commit if the others are too fixup-y. ACKs for top commit: laanwj: ACK 6659810 Tree-SHA512: bf5258f23802ab3ba3defb8791097e08e63f3e2af21023f832cd270dc88d1fa04349e921d69f9f5fedac5dce5cd3c1cc46b48febbede4bc18dccb8be994565b2

… as BTC/kB 6659810 test: use named args for sendrawtransaction calls (Jon Atack) 5c1cd78 doc: improve rawtransaction code/test docs (Jon Atack) acc14c5 test: fix incorrect value in rpc_rawtransaction.py (Jon Atack) Pull request description: Follow-up to PR bitcoin#16521. - Fix incorrect value in rpc_rawtransaction test as per https://github.com/bitcoin/bitcoin/pull/16521/files#r325842308 - Improve the code docs - Use named arguments as per https://github.com/bitcoin/bitcoin/pull/16521/files#r310715127 Happy to squash or keep only the first commit if the others are too fixup-y. ACKs for top commit: laanwj: ACK 6659810 Tree-SHA512: bf5258f23802ab3ba3defb8791097e08e63f3e2af21023f832cd270dc88d1fa04349e921d69f9f5fedac5dce5cd3c1cc46b48febbede4bc18dccb8be994565b2

Summary: * wallet/rpc: Use the default maxfeerate value as BTC/kB * test: Add test for default maxfeerate in sendrawtransaction This patch adds test for the following case - maxfeerate is omitted, and the actual fee rate is higher than the default value(0.1 BTC/kB) This is a backport of Core [[bitcoin/bitcoin#16521 | PR16521]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8027

maflcko approved these changes Aug 1, 2019

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

test/functional/rpc_rawtransaction.py Outdated Show resolved Hide resolved

test/functional/rpc_rawtransaction.py Show resolved Hide resolved

DrahtBot added RPC/REST/ZMQ Tests labels Aug 1, 2019

remagpie force-pushed the maxfeerate-as-rate branch from 5665a1a to 31ad961 Compare August 1, 2019 19:16

maflcko reviewed Aug 1, 2019

View reviewed changes

test/functional/rpc_rawtransaction.py Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Aug 2, 2019

maflcko added this to the 0.19.0 milestone Aug 2, 2019

remagpie force-pushed the maxfeerate-as-rate branch 2 times, most recently from 0bb71b1 to c540e25 Compare August 3, 2019 03:31

DrahtBot removed the Needs rebase label Aug 3, 2019

maflcko mentioned this pull request Aug 5, 2019

wallet: lower -txmaxfee default from 0.1 to 0.01 BTC #16539

Closed

maflcko reviewed Aug 5, 2019

View reviewed changes

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

src/rpc/rawtransaction.cpp Outdated Show resolved Hide resolved

test/functional/feature_segwit.py Show resolved Hide resolved

test/functional/wallet_basic.py Show resolved Hide resolved

remagpie force-pushed the maxfeerate-as-rate branch from 8153e7f to 7335bd3 Compare August 5, 2019 19:15

maflcko changed the title ~~wallet/rpc: Use the default maxfeerate value as BTC/kB~~ rpc: Use the default maxfeerate value as BTC/kB Aug 6, 2019

kallewoof reviewed Aug 7, 2019

View reviewed changes

remagpie force-pushed the maxfeerate-as-rate branch from 7335bd3 to 49b1f48 Compare August 7, 2019 14:24

practicalswift reviewed Aug 18, 2019

View reviewed changes

remagpie added 2 commits August 18, 2019 22:58

wallet/rpc: Use the default maxfeerate value as BTC/kB

261843e

test: Add test for default maxfeerate in sendrawtransaction

2dfd683

This patch adds test for the following case - maxfeerate is omitted, and the actual fee rate is higher than the default value(0.1 BTC/kB)

remagpie force-pushed the maxfeerate-as-rate branch from 49b1f48 to 2dfd683 Compare August 18, 2019 15:16

maflcko closed this Sep 16, 2019

maflcko reopened this Sep 16, 2019

laanwj merged commit 2dfd683 into bitcoin:master Sep 18, 2019

maflcko reviewed Sep 18, 2019

View reviewed changes

jonatack mentioned this pull request Sep 21, 2019

test: follow-up to rpc: default maxfeerate value as BTC/kB #16929

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

rpc: Use the default maxfeerate value as BTC/kB #16521

rpc: Use the default maxfeerate value as BTC/kB #16521

Uh oh!

Conversation

remagpie commented Aug 1, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Aug 2, 2019

Uh oh!

promag commented Aug 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 2, 2019

Uh oh!

maflcko commented Aug 2, 2019

Uh oh!

promag commented Aug 2, 2019

Uh oh!

maflcko commented Aug 2, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Aug 5, 2019

Uh oh!

remagpie commented Aug 6, 2019

Uh oh!

Sjors commented Aug 6, 2019

Uh oh!

maflcko commented Aug 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

remagpie commented Aug 7, 2019

Uh oh!

maflcko commented Aug 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 16, 2019

Uh oh!

remagpie commented Sep 17, 2019

Uh oh!

laanwj commented Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Aug 1, 2019 •

edited

Loading

promag commented Aug 2, 2019 •

edited

Loading

laanwj commented Sep 18, 2019 •

edited

Loading