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

pull promag wants to merge 3 commits into bitcoin:master from promag:2019-07-fix-16257 changing 5 files +60 −16
  1. 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.

  2. promag commented at 2:29 PM on July 2, 2019: member

    @Sjors you promised a review 😄

  3. 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?

  4. 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:

        if (nFeeRet > m_default_max_tx_fee) {

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

  5. 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?

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

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

  8. wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction 5c1b9714cb
  9. promag force-pushed on Jul 2, 2019
  10. wallet: Remove unreachable code in CreateTransaction 177550101b
  11. MarcoFalke commented at 3:52 PM on July 2, 2019: member

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

  12. MarcoFalke added this to the milestone 0.18.1 on Jul 2, 2019
  13. fanquake added the label Wallet on Jul 2, 2019
  14. fanquake added the label Needs backport on Jul 2, 2019
  15. 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.

  16. promag commented at 6:55 PM on July 2, 2019: member

    @MarcoFalke great stuff, pulled both commits.

  17. DrahtBot commented at 7:32 PM on July 2, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  18. MarcoFalke commented at 8:17 PM on July 2, 2019: member

    ACK 9ff66a1b2157bcce8ccd4cea0610eae79d0be6d8 (read code, wrote test case)

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 9ff66a1b2157bcce8ccd4cea0610eae79d0be6d8 (read code, wrote test case)
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhCrQv8C829o7ZZs8hUWaldWPLHQdivLOKMhyEC24GVjlOb2CPwh+YDSs4bZUNj
qVpgveclR55eb9j4tj7Wt+4AqNiNeHyKMgNfXCXeWKj4CYDSGRfwd1zpSnL8Z/Lk
cXyp2AV5GcSX3hjG/1tbFphxDYLoiihfSLMVTGybC4aNupxqIiAFRPBVS9ovwHBA
Ml3r9H0CueguuZMR1m2B/hxA+fOWe19VxCck93dydfL7v+N56tvatm/+5ihwaXAf
v3wwrHHdA2j0i3yXOSCdDezC0arwA3CMVJWZNt5rJt2ydLG46YSp7S4XwjpJD36V
G04nwXgfIRv9HPPeYLESOQHf/RiGQaaC4TRLuxPo3P/abhzXq2ucSTdsm0mtqeuj
pTdasa8y6fAWs4Y//y84ce28bbLpWwM1QHCbf1GwemqNhXpxOwZnk6uAqwM+2D6v
SJ0hV5HqocIfbUM1WKqBnKpLnvHicvPeMOjTtQw3g1KHvhY9DCK9tAX1vYom2qDd
Y5xHfFCm
=I+BI
-----END PGP SIGNATURE-----

    Timestamp of file with hash e8c4afba47440f5e82e36c70bed731c61cc784c18d70774903ee95050752dbfa -

    </details>

  19. laanwj commented at 1:12 PM on July 3, 2019: member

    Concept ACK

  20. promag commented at 9:12 AM on July 8, 2019: member

    @meshcollider ping.

  21. fanquake requested review from meshcollider on Jul 8, 2019
  22. 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.

  23. kallewoof approved
  24. 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.

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

  26. test: Add test for maxtxfee option 0d101a340c
  27. promag force-pushed on Jul 10, 2019
  28. MarcoFalke commented at 11:56 AM on July 10, 2019: member

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

    <details><summary>Show signature and timestamp</summary>

    Signature:

    -----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

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

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgQbwwAtg+GMzWeDLh1+NCCGz5Bs6tmgzthA2R28KiIaoQsgivJMdd2FoOMl7/C
dlKZ7gKbttnEtr/Rl4pFALzy9L8oMiFmoXC+qy3APx4Z3Xd+MS4aLM1WqxHggkPN
u6679W5/OtN4UE6oksKGM84EOAfKqSCHG/cZqGJ8a4UCIZWku2EhHiqo16gaNJvi
5MNuaIQjpGqQf5egbqV+lWIvJmnHXzoIKN09Eo7PS7PwtCJy6A1x8uy12CxsU6G3
Dl8M27a52PNvoXHeWX+xyKlbDIFSOf9vuc4/bP/ylMtC7vIGdO/W0SIyvsaNMLHq
+cOAJZxLoxZKgK2VUQqxH1L/VafPtm/a2v23LzkUFoK7Vk0fENekpuiQTZsDRQV8
OOXhf7TFukNGVCS60tBJa1Mf0LrSfT3DgV/NNVni1YYDg97H6tX0lpXQyJ8LSZg5
o8dWBno9MWsoFAyAfsPxWtS9FI7xLb1XuYMiMNBdF2FGikHIebt9R5CJqfGVIgoc
9IJFNrPA
=Cuz4
-----END PGP SIGNATURE-----

    Timestamp of file with hash a2d0938771647864e65f9d3c8939d69334e4a3be00ac928e24814e25e15140d2 -

    </details>

  29. laanwj commented at 11:59 AM on July 10, 2019: member

    ACK 0d101a340c44841cbbc5982d55354b1787bc39e2

  30. laanwj merged this on Jul 10, 2019
  31. laanwj closed this on Jul 10, 2019
  32. laanwj referenced this in commit 6c1e45c4c4 on Jul 10, 2019
  33. sidhujag referenced this in commit 6a23f381b5 on Jul 10, 2019
  34. fanquake removed the label Needs backport on Jul 25, 2019
  35. fanquake commented at 3:21 AM on July 25, 2019: member

    Being backported in #16414.

  36. Sjors referenced this in commit 0e7c7465bf on Aug 19, 2019
  37. promag referenced this in commit a11dbaa547 on Aug 25, 2019
  38. promag referenced this in commit d3b3bb8c9f on Aug 25, 2019
  39. MarcoFalke referenced this in commit 3b04221183 on Sep 23, 2019
  40. deadalnix referenced this in commit 355883604d on Jun 22, 2020
  41. Munkybooty referenced this in commit 2a6ddb4baf on Nov 4, 2021
  42. Munkybooty referenced this in commit 8f421a49fc on Nov 4, 2021
  43. DrahtBot locked this on Dec 16, 2021
Contributors
promagSjorsMarcoFalkeDrahtBotlaanwjkallewoofpracticalswiftfanquake


meshcollider

Labels
Wallet

Milestone
0.18.1

Linked (view graph)
#16257 [wallet] abort when attempting to fund a transaction above -maxtxfee#16414 0.18: wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 18:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me