Skip to content

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

Merged
laanwj merged 3 commits intobitcoin:masterfrom
promag:2019-07-fix-16257
Jul 10, 2019
Merged

wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction#16322
laanwj merged 3 commits intobitcoin:masterfrom
promag:2019-07-fix-16257

Conversation

@promag
Copy link
Contributor

@promag promag commented Jul 2, 2019

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
Copy link
Contributor Author

promag commented Jul 2, 2019

@Sjors you promised a review 😄

@Sjors
Copy link
Member

Sjors commented Jul 2, 2019

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?

maflcko
maflcko reviewed Jul 2, 2019
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maflcko
Copy link
Member

maflcko commented Jul 2, 2019

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

@promag promag force-pushed the 2019-07-fix-16257 branch from a2f23b8 to 5c1b971 Compare July 2, 2019 15:13
@maflcko
Copy link
Member

maflcko commented Jul 2, 2019

ACK, tested by writing a test: maflcko@9ff66a1

@maflcko maflcko added this to the 0.18.1 milestone Jul 2, 2019
@Sjors
Copy link
Member

Sjors commented Jul 2, 2019

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
Copy link
Contributor Author

promag commented Jul 2, 2019

@MarcoFalke great stuff, pulled both commits.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 2, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@maflcko
Copy link
Member

maflcko commented Jul 2, 2019

ACK 9ff66a1 (read code, wrote test case)

Show signature and timestamp

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 -

@laanwj
Copy link
Member

laanwj commented Jul 3, 2019

Concept ACK

@promag
Copy link
Contributor Author

promag commented Jul 8, 2019

@meshcollider ping.

@fanquake fanquake requested a review from meshcollider July 8, 2019 10:14
@maflcko
Copy link
Member

maflcko commented Jul 8, 2019

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

kallewoof
kallewoof approved these changes Jul 9, 2019
View reviewed changes
Copy link
Contributor

@kallewoof kallewoof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 9ff66a1

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

practicalswift
practicalswift reviewed Jul 10, 2019
View reviewed changes
test/functional/wallet_create_tx.py Outdated
lambda: self.nodes[0].fundrawtransaction(hexstring=raw_tx),
)

self.log.info('Check maxtxfee in combination with settxfee'.format(fee_setting))
Copy link
Contributor

@practicalswift practicalswift Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@promag promag Jul 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yap, @MarcoFalke I'll amend.

@promag promag force-pushed the 2019-07-fix-16257 branch from 9ff66a1 to 0d101a3 Compare July 10, 2019 10:59
@maflcko
Copy link
Member

maflcko commented Jul 10, 2019

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

Show signature and timestamp

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 -

@laanwj
Copy link
Member

laanwj commented Jul 10, 2019

ACK 0d101a3

@laanwj laanwj merged commit 0d101a3 into bitcoin:master Jul 10, 2019
laanwj added a commit that referenced this pull request Jul 10, 2019
@laanwj
Merge #16322: wallet: Fix -maxtxfee check by moving it to CWallet::Cr…
6c1e45c 
…eateTransaction

0d101a3 test: Add test for maxtxfee option (MarcoFalke)
1775501 wallet: Remove unreachable code in CreateTransaction (MarcoFalke)
5c1b971 wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (João Barbosa)

Pull request description:

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

ACKs for top commit:
  MarcoFalke:
    re-ACK 0d101a3, only change is small test fixup

Tree-SHA512: 3464b24ae7cd4e72ed41438c6661828ba1304af020f05da62720b23668ae734e16cf47c6d97e150cc84ef631ee099b16fc786c858f3d089905845437338fd512
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 10, 2019
@laanwj @sidhujag
Merge bitcoin#16322: wallet: Fix -maxtxfee check by moving it to CWal…
6a23f38 
…let::CreateTransaction

0d101a3 test: Add test for maxtxfee option (MarcoFalke)
1775501 wallet: Remove unreachable code in CreateTransaction (MarcoFalke)
5c1b971 wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (João Barbosa)

Pull request description:

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

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

ACKs for top commit:
  MarcoFalke:
    re-ACK 0d101a3, only change is small test fixup

Tree-SHA512: 3464b24ae7cd4e72ed41438c6661828ba1304af020f05da62720b23668ae734e16cf47c6d97e150cc84ef631ee099b16fc786c858f3d089905845437338fd512
@promag promag mentioned this pull request Jul 18, 2019
Merged
@fanquake
Copy link
Member

fanquake commented Jul 25, 2019

Being backported in #16414.

Sjors pushed a commit to Sjors/bitcoin that referenced this pull request Aug 19, 2019
@promag @Sjors
wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction
0e7c746 
Github-Pull: bitcoin#16322
Rebased-From: 5c1b971
promag added a commit to promag/bitcoin that referenced this pull request Aug 25, 2019
@promag
0.18: wallet: Fix -maxtxfee check by moving it to CWallet::CreateTran…
a11dbaa 
…saction

Github-Pull: bitcoin#16322
Rebased-From: 5c1b971
promag pushed a commit to promag/bitcoin that referenced this pull request Aug 25, 2019
@promag
0.18: test: Add test for maxtxfee option
d3b3bb8 
Github-Pull: bitcoin#16322
Rebased-From: 0d101a3
maflcko pushed a commit that referenced this pull request Sep 23, 2019
Merge #16414: 0.18: wallet: Fix -maxtxfee check by moving it to CWall…
3b04221 
…et::CreateTransaction

d3b3bb8 0.18: test: Add test for maxtxfee option (MarcoFalke)
a11dbaa 0.18: wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (João Barbosa)
8f354ce 0.18: [wallet] abort when attempting to fund a transaction above maxtxfee (Sjors Provoost)

Pull request description:

  Backports #16322 and #16257.

ACKs for top commit:
  Sjors:
    re-ACK d3b3bb8
  MarcoFalke:
    ACK d3b3bb8 (did the backport myself, arrived at the same result)

Tree-SHA512: 8b0ca15fc7e893af80239afecf8ff1018d6f249f2fa530babe61ec34ede6103b9b60909259abb730ebf1d54789aceed94b136600158dc3d6c5505b07f28189e5
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 22, 2020
@promag @majcosta
[backport#16322] wallet: Fix -maxtxfee check by moving it to CWallet:…
3558836 
…:CreateTransaction

Summary:
wallet: Remove unreachable code in CreateTransaction
test: Add test for maxtxfee option

---

Backport of Core [[bitcoin/bitcoin#16322 | PR16322]]

Test Plan:
  ninja all check
  ./test/functional/test_runner.py wallet_create_tx

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D6664
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@meshcollider meshcollider Awaiting requested review from meshcollider

3 more reviewers

@maflcko maflcko maflcko left review comments

@practicalswift practicalswift practicalswift left review comments

@kallewoof kallewoof kallewoof approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Wallet

Projects

None yet

Milestone

0.18.1

Development

Successfully merging this pull request may close these issues.

8 participants

@promag @Sjors @maflcko @DrahtBot @laanwj @fanquake @kallewoof @practicalswift