Skip to content

wallet: ensure sat/vB feerates are in range (mantissa of 3) #21786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
May 10, 2021
Merged

wallet: ensure sat/vB feerates are in range (mantissa of 3) #21786

merged 7 commits into from
May 10, 2021

Conversation

jonatack
Copy link
Member

  • Improve/close gaps in existing test coverage before making the change
  • Enable passing decimals to ParseFixedPoint() when calling AmountFromValue()
  • Limit explicit fee rates in sat/vB passed in by users to 3 decimals, and raise otherwise
  • Add regression test coverage

Closes #20534.

This was referenced Apr 27, 2021
Closed
Closed
jonatack
jonatack commented Apr 27, 2021
View reviewed changes
@maflcko
Copy link
Member

maflcko commented Apr 27, 2021

Concept ACK

@DrahtBot DrahtBot mentioned this pull request Apr 27, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 27, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot DrahtBot mentioned this pull request Apr 27, 2021
Closed
@jonatack jonatack force-pushed the ensure-sat-vb-feerates-are-in-range branch from 8acc7f9 to 9580bd8 Compare April 28, 2021 20:05
@DrahtBot DrahtBot mentioned this pull request Apr 30, 2021
Merged
@jonatack jonatack force-pushed the ensure-sat-vb-feerates-are-in-range branch from 9580bd8 to a224f52 Compare May 5, 2021 13:44
@jonatack
Copy link
Member Author

jonatack commented May 5, 2021

rebased

@jonatack jonatack force-pushed the ensure-sat-vb-feerates-are-in-range branch from a224f52 to 9c7b474 Compare May 6, 2021 07:06
@jonatack
Copy link
Member Author

jonatack commented May 6, 2021

Rebased for CI fix on master.

@meshcollider
Copy link
Contributor

Concept ACK

fjahr
fjahr reviewed May 8, 2021
View reviewed changes
Copy link
Contributor

@fjahr fjahr left a comment

Choose a reason for hiding this comment

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

Code review ACK 9c7b474

modulo I think those two tests mentioned below can be removed now.

@jonatack jonatack force-pushed the ensure-sat-vb-feerates-are-in-range branch from 9c7b474 to 847288d Compare May 9, 2021 10:51
@jonatack
Copy link
Member Author

jonatack commented May 9, 2021

Thank you @fjahr, updated with your feedback: git diff 9c7b474 847288d

@fjahr
Copy link
Contributor

fjahr commented May 9, 2021

Code review ACK 9c7b474

The only change was the removal of two now redundant/duplicate tests.

maflcko
maflcko approved these changes May 10, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

review ACK 847288d 🔷

Show signature and timestamp

Signature:

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

review ACK 847288df07b45ca535c849e518b22818ab492896 🔷
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjgOAv/U64jIK5O7lauqHgzSxVR8ARVhocyv95mL70s0DRDSUXyWeNjsQAbqq4d
d/Frd1URcN+ui/wiGAvzW7JjgPxdFBfUpcUZCh6uVwzMYIi5JfzDpVVC1Fa/S9PG
Wcvi14s/N7wWMEYRE85WMncceEyCAb5NP6/Pnr8zY3/jdMT+NGZv+OvIkNgpxpFt
CP9iMkX5GFuHOJnFc53PNAvvuGnoDE/jGWDk7X48Q8EScNgwKnk16K7vNDbarKb2
szmXO2H+ql7moWoTBK0xDr/X2GEdZw+49nr1aP11pxBAIbgk0T/PweW6HQlnKaQ4
Ys/mVrSS18laMZkK2LvOgHQynSHmwwUyekcCzaxNWkRP2Si17bIwWlCCswPL4L1Z
qgWDQomJ2eJoYUBJMFwxTe0ZqVlRNEQA3ah5dv9qmImo9Qezf28E5fHeZ/OmHiEo
dBECvDU5QZlwFsUZ94kRY7t0/8E+5yGDESsmYV3120BkOYLcGx+OARlT0YZcyRaz
7SjbaQL7
=gqY6
-----END PGP SIGNATURE-----

Timestamp of file with hash 8b19ebaa4f993d9f7f5a3cd958924c1f48d2c00e94f5d00a4b4b7012058dc816 -

test/functional/rpc_fundrawtransaction.py
assert_fee_amount(result6['fee'], count_bytes(result6['hex']), 0)

# Test that funding non-standard "zero-fee" transactions is valid.
for param, zero_value in product(["fee_rate", "feeRate"], [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]):
Copy link
Member

@maflcko maflcko May 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There should be no difference in python between 0., and .0, or any other combination that appends zeros to any side of either of them.

@maflcko maflcko merged commit adf7843 into bitcoin:master May 10, 2021
@jonatack jonatack deleted the ensure-sat-vb-feerates-are-in-range branch May 10, 2021 07:18
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 10, 2021
Merge bitcoin#21786: wallet: ensure sat/vB feerates are in range (man…
2822599 
…tissa o…
@jonatack jonatack mentioned this pull request Jul 13, 2021
Closed
@jonatack jonatack mentioned this pull request Sep 29, 2021
Open
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge adf7843 into merged_master (Bitcoin PR bitcoin/bitcoin#21786)
9c4837d
@adamjonas adamjonas mentioned this pull request Aug 2, 2022
Closed
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
2 more reviewers

@fjahr fjahr fjahr left review comments

@maflcko maflcko maflcko approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
RPC/REST/ZMQ Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

sat/b values aren't validated to be in-range
5 participants
@jonatack @maflcko @DrahtBot @meshcollider @fjahr