wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction

promag commented at 2:23 pm on July 2, 2019: member

Follow up to #16257, this PR makes bumpfee aware of -maxtxfee.

It also prevents dangling locked unspents when calling fundrawtransaction - because the previous check was after LockCoin.

promag commented at 2:29 pm on July 2, 2019: member

@Sjors you promised a review 😄

Sjors commented at 2:40 pm on July 2, 2019: member

tACK a2f23b8

This also makes the QT message the same as RPC.

Afaik there’s no way to reach the A fee higher than %1 is considered an absurdly high fee message anymore, so should we get rid of that string and Reject absurdly high fee in walletmodel.cpp?

in src/wallet/wallet.cpp:3133 in a2f23b810d outdated

3129@@ -3135,6 +3130,11 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
3130         }
3131     }
3132 
3133+    if (nFeeRet > this->m_default_max_tx_fee) {

MarcoFalke commented at 2:47 pm on July 2, 2019:

style-nit:

0    if (nFeeRet > m_default_max_tx_fee) {

m_ already means this->, no need for duplicate.

in src/wallet/wallet.cpp:3134 in a2f23b810d outdated

3129@@ -3135,6 +3130,11 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
3130         }
3131     }
3132 
3133+    if (nFeeRet > this->m_default_max_tx_fee) {
3134+        strFailReason = TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED);

MarcoFalke commented at 2:48 pm on July 2, 2019:

style-nit: Maybe for the purpose of a backport, this could be inlined as a string literal?

promag commented at 3:15 pm on July 2, 2019:

Don’t you prefer an unclean-but-easy-cherry-pick backport?

MarcoFalke commented at 2:49 pm on July 2, 2019: member

Concept ACK.

Maybe revert the docstring in the gui to say “belt-and-suspenders” again?

See https://github.com/bitcoin/bitcoin/pull/16257/files#diff-2e3836af182cfb375329c3463ffd91f8

MarcoFalke commented at 2:51 pm on July 2, 2019: member

Looks good, I’ll write some tests and send them over in a bit.

wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction 5c1b9714cb

promag force-pushed on Jul 2, 2019

wallet: Remove unreachable code in CreateTransaction 177550101b

MarcoFalke commented at 3:52 pm on July 2, 2019: member

ACK, tested by writing a test: https://github.com/MarcoFalke/bitcoin/commit/9ff66a1b2157bcce8ccd4cea0610eae79d0be6d8

MarcoFalke added this to the milestone 0.18.1 on Jul 2, 2019

fanquake added the label Wallet on Jul 2, 2019

fanquake added the label Needs backport on Jul 2, 2019

Sjors commented at 6:52 pm on July 2, 2019: member

Thanks for the sendmany and fundrawtransaction RPC tests @MarcoFalke.

Maybe revert the docstring in the gui to say “belt-and-suspenders” again?

Maybe that is indeed slightly safer than dropping it completely.

promag commented at 6:55 pm on July 2, 2019: member

@MarcoFalke great stuff, pulled both commits.

DrahtBot commented at 7:32 pm on July 2, 2019: member

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

Conflicts

No conflicts as of last run.

MarcoFalke commented at 8:17 pm on July 2, 2019: member

ACK 9ff66a1b2157bcce8ccd4cea0610eae79d0be6d8 (read code, wrote test case)

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 9ff66a1b2157bcce8ccd4cea0610eae79d0be6d8 (read code, wrote test case)
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhCrQv8C829o7ZZs8hUWaldWPLHQdivLOKMhyEC24GVjlOb2CPwh+YDSs4bZUNj
 8qVpgveclR55eb9j4tj7Wt+4AqNiNeHyKMgNfXCXeWKj4CYDSGRfwd1zpSnL8Z/Lk
 9cXyp2AV5GcSX3hjG/1tbFphxDYLoiihfSLMVTGybC4aNupxqIiAFRPBVS9ovwHBA
10Ml3r9H0CueguuZMR1m2B/hxA+fOWe19VxCck93dydfL7v+N56tvatm/+5ihwaXAf
11v3wwrHHdA2j0i3yXOSCdDezC0arwA3CMVJWZNt5rJt2ydLG46YSp7S4XwjpJD36V
12G04nwXgfIRv9HPPeYLESOQHf/RiGQaaC4TRLuxPo3P/abhzXq2ucSTdsm0mtqeuj
13pTdasa8y6fAWs4Y//y84ce28bbLpWwM1QHCbf1GwemqNhXpxOwZnk6uAqwM+2D6v
14SJ0hV5HqocIfbUM1WKqBnKpLnvHicvPeMOjTtQw3g1KHvhY9DCK9tAX1vYom2qDd
15Y5xHfFCm
16=I+BI
17-----END PGP SIGNATURE-----

Timestamp of file with hash e8c4afba47440f5e82e36c70bed731c61cc784c18d70774903ee95050752dbfa -

laanwj commented at 1:12 pm on July 3, 2019: member

Concept ACK

promag commented at 9:12 am on July 8, 2019: member

@meshcollider ping.

fanquake requested review from meshcollider on Jul 8, 2019

MarcoFalke commented at 5:13 pm on July 8, 2019: member

The previous pull was reviewed by @kallewoof @jonasschnelli @laanwj @achow101. Would be helpful to get your eyes on this as well.

kallewoof approved

kallewoof commented at 1:54 am on July 9, 2019: member

utACK 9ff66a1b2157bcce8ccd4cea0610eae79d0be6d8

Looks good. Was wondering about the two test commits, until I realized author was different.

in test/functional/wallet_create_tx.py:65 in 9ff66a1b21 outdated

60+                -4,
61+                "Fee exceeds maximum configured by -maxtxfee",
62+                lambda: self.nodes[0].fundrawtransaction(hexstring=raw_tx),
63+            )
64+
65+        self.log.info('Check maxtxfee in combination with settxfee'.format(fee_setting))

practicalswift commented at 10:23 am on July 10, 2019:

.format(fee_setting) part is a mistake? :-)

promag commented at 10:51 am on July 10, 2019:

Yap, @MarcoFalke I’ll amend.

test: Add test for maxtxfee option 0d101a340c

promag force-pushed on Jul 10, 2019

MarcoFalke commented at 11:56 am on July 10, 2019: member

re-ACK 0d101a340c44841cbbc5982d55354b1787bc39e2, only change is small test fixup

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3re-ACK 0d101a340c44841cbbc5982d55354b1787bc39e2, only change is small test fixup
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgQbwwAtg+GMzWeDLh1+NCCGz5Bs6tmgzthA2R28KiIaoQsgivJMdd2FoOMl7/C
 8dlKZ7gKbttnEtr/Rl4pFALzy9L8oMiFmoXC+qy3APx4Z3Xd+MS4aLM1WqxHggkPN
 9u6679W5/OtN4UE6oksKGM84EOAfKqSCHG/cZqGJ8a4UCIZWku2EhHiqo16gaNJvi
105MNuaIQjpGqQf5egbqV+lWIvJmnHXzoIKN09Eo7PS7PwtCJy6A1x8uy12CxsU6G3
11Dl8M27a52PNvoXHeWX+xyKlbDIFSOf9vuc4/bP/ylMtC7vIGdO/W0SIyvsaNMLHq
12+cOAJZxLoxZKgK2VUQqxH1L/VafPtm/a2v23LzkUFoK7Vk0fENekpuiQTZsDRQV8
13OOXhf7TFukNGVCS60tBJa1Mf0LrSfT3DgV/NNVni1YYDg97H6tX0lpXQyJ8LSZg5
14o8dWBno9MWsoFAyAfsPxWtS9FI7xLb1XuYMiMNBdF2FGikHIebt9R5CJqfGVIgoc
159IJFNrPA
16=Cuz4
17-----END PGP SIGNATURE-----

Timestamp of file with hash a2d0938771647864e65f9d3c8939d69334e4a3be00ac928e24814e25e15140d2 -

laanwj commented at 11:59 am on July 10, 2019: member

ACK 0d101a340c44841cbbc5982d55354b1787bc39e2

laanwj merged this on Jul 10, 2019

laanwj closed this on Jul 10, 2019

laanwj referenced this in commit 6c1e45c4c4 on Jul 10, 2019

sidhujag referenced this in commit 6a23f381b5 on Jul 10, 2019

fanquake removed the label Needs backport on Jul 25, 2019

fanquake commented at 3:21 am on July 25, 2019: member

Being backported in #16414.

Sjors referenced this in commit 0e7c7465bf on Aug 19, 2019

promag referenced this in commit a11dbaa547 on Aug 25, 2019

promag referenced this in commit d3b3bb8c9f on Aug 25, 2019

MarcoFalke referenced this in commit 3b04221183 on Sep 23, 2019

deadalnix referenced this in commit 355883604d on Jun 22, 2020

Munkybooty referenced this in commit 2a6ddb4baf on Nov 4, 2021

Munkybooty referenced this in commit 8f421a49fc on Nov 4, 2021

DrahtBot locked this on Dec 16, 2021

wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction #16322

Conflicts