Skip to content
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.

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

rpc: calculate fees in getblock using BlockUndo data #18772

Merged
merged 2 commits into from Dec 24, 2020

Conversation

robot-visions
Copy link
Contributor

@robot-visions robot-visions commented Apr 26, 2020

This change is progress towards #18771 . It adapts the fee calculation part of #16083 and addresses some feedback. The additional "verbosity level 3" features are planned for a future PR.

Original PR description:

Using block undo data (like in #14802) we can now show fee information for each transaction in a block without the need for additional -txindex and/or a ton of costly lookups. For a start we'll add transaction fee information to getblock verbosity level 2. This comes at a negligible speed penalty (<1%).

@robot-visions robot-visions changed the title rpc: calculate fee in getblock using BlockUndo data rpc: calculate fees in getblock using BlockUndo data Apr 26, 2020
@robot-visions
Copy link
Contributor Author

robot-visions commented Apr 26, 2020

The documentation for getblock refers to the tx format of getrawtransaction. Any thoughts on what would be a reasonable approach here?

  • Also update getrawtransaction (and the documentation) to include a fee if possible (this might be slow since the cost of loading undo data is no longer amortized over many transactions in a block)
  • Update the getblock documentation to mention that unlike getrawtransaction, it might also include fees
  • Leave the documentation alone and let the unfortunate user figure out the semantics themselves :)

@MarcoFalke
Copy link
Member

MarcoFalke commented Apr 26, 2020

Please add the right co-author to your commit, if your work is based on someone else's work

@robot-visions
Copy link
Contributor Author

robot-visions commented Apr 26, 2020

Updated, thanks! I didn't know about Co-authored-by feature before.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 30, 2020

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.

@brakmic
Copy link
Contributor

brakmic commented May 1, 2020

ACK 732554a

Built, run and tested on macOS Catalina 10.15.4

Copy link
Member

@luke-jr luke-jr left a comment

Looks like it was based on the original PR rather than my rebase, and therefore missed some fixups.

src/core_write.cpp Outdated Show resolved Hide resolved
src/core_write.cpp Outdated Show resolved Hide resolved
src/core_write.cpp Outdated Show resolved Hide resolved
src/core_write.cpp Outdated Show resolved Hide resolved
src/core_write.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
@luke-jr
Copy link
Member

luke-jr commented May 10, 2020

Proposed update: master...luke-jr:getblock_fees-0.20

@robot-visions
Copy link
Contributor Author

robot-visions commented May 10, 2020

Thanks for the review and the update @luke-jr ! I'm happy to go with your proposed update (we can close this PR).

Quick notes:

  • I didn't include the verbosity 3 changes because I wanted to move things over incrementally
  • In your updates it might still be necessary to introduce some way to prevent the fuzz tests from failing (it was causing an integer overflow when adding a large value to amt_total_out); I added a halfhearted check (which you correctly pointed out doesn't always work) but maybe it makes more sense to just update the fuzz test

@luke-jr
Copy link
Member

luke-jr commented May 14, 2020

We don't need to close this PR, you're doing fine.

If you want to adopt my branch as-is, you can just checkout yours, git reset --hard 0b46eb7ac63, and re-push it here.

test/functional/rpc_getblock_fee.py Outdated Show resolved Hide resolved
src/core_write.cpp Outdated Show resolved Hide resolved
Copy link
Member

@jnewbery jnewbery left a comment

Looks good. It'd be nice if the test checked that the fee returned was correct rather than just testing that any value is returned.

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
test/functional/rpc_getblock_fee.py Outdated Show resolved Hide resolved
test/functional/rpc_getblock_fee.py Outdated Show resolved Hide resolved
src/core_write.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@fjahr fjahr left a comment

Concept ACK

src/core_write.cpp Outdated Show resolved Hide resolved
src/core_write.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Show resolved Hide resolved
test/functional/rpc_getblock_fee.py Outdated Show resolved Hide resolved
test/functional/rpc_getblock_fee.py Outdated Show resolved Hide resolved
@troygiorshev
Copy link
Contributor

troygiorshev commented Sep 30, 2020

Approach ACK.

Reviewed changes, ran tests. Need to manually test further. 💻

I agree with the previous review comments.

Feel free to add a commit at the beginning cleaning up the code around your changes!
For example:

@Emzy
Copy link
Contributor

Emzy commented Oct 1, 2020

I get following error:

$ bitcoin-cli getblock "$(bitcoin-cli getblockhash 650684)" 2
error code: -1
error message:
core_write.cpp:251 (TxToUniv)
Internal bug detected: 'MoneyRange(fee)'
You may report this issue here: https://github.com/bitcoin/bitcoin/issues

A block with only a Coinbase transaction works, for example 650026.

@Emzy
Copy link
Contributor

Emzy commented Oct 2, 2020

I get following error:

$ bitcoin-cli getblock "$(bitcoin-cli getblockhash 650684)" 2
error code: -1
error message:
core_write.cpp:251 (TxToUniv)
Internal bug detected: 'MoneyRange(fee)'
You may report this issue here: https://github.com/bitcoin/bitcoin/issues

A block with only a Coinbase transaction works, for example 650026.

8a7229e fixed it.

@robot-visions robot-visions force-pushed the getblock-fee branch 2 times, most recently from 3f0490e to 8ccfbde Compare Oct 3, 2020
Copy link
Contributor

@fjahr fjahr left a comment

utACK 8ccfbde

Feel free to ignore my comments unless you have to make other changes.

src/core_write.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
src/core_write.cpp Outdated Show resolved Hide resolved
src/rpc/blockchain.cpp Outdated Show resolved Hide resolved
test/functional/rpc_blockchain.py Outdated Show resolved Hide resolved
test/functional/rpc_blockchain.py Outdated Show resolved Hide resolved
@jnewbery
Copy link
Member

jnewbery commented Oct 9, 2020

utACK dd4ec07

Since the first commit is very different from the version in 16083, you should feel free to set yourself as the commit author and add felix as a co-author (or just add yourself as co-author).

Also feel free to set the review comments above as 'resolved' if you think they're addressed. I went through and resolved my own review comments, but didn't resolve comments from other reviewers. Setting all comments to resolved signals to other reviewers that you've addressed all the outstanding issues.

@luke-jr
Copy link
Member

luke-jr commented Nov 8, 2020

tACK 66d012a

@fjahr
Copy link
Contributor

fjahr commented Dec 23, 2020

tACK 66d012a

Changes since last review were a small refactoring of the functional test using MiniWallet and some improvements on comments/docs.

Copy link
Member

@MarcoFalke MarcoFalke left a comment

Untitled pnggg

review ACK 66d012a 🗜

Show signature and timestamp

Signature:

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

review ACK 66d012ad7f9381bacfc9b8388fa2ebf82cb86c9e 🗜
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUiUbgwAzPzceMugFaUVExZY36XvjsXenEtSk7a/xjrn8+SkZOd1gPtb852CYkQF
CuWb42KnECcFnSw+BvesBuHqn6lcOdHlD8yqNaA3IJdW1UJREN0ik8rScuBnNigS
7PrsiSJVESlXP16YcvlcFl/z/OmZ15tY+p+6IIed5/tp1CC5SYvkx2qUgLFXomU3
pbiuzE6rePULNjthMcS+CO8/8id2hPT4UoJdRxEZJ5pAqaIAtdwe0XFAUEFt6jwM
2gkafeYnfk3c/lz/oIschVRrwFI/Hff7H/yuVPDzXebzQvHyMOATSyguB4xSVtyD
tXOUlVovzdCkm6pTXn2gFLm59NbPDW0VbfJcFrhTEFdFV70MQ3scqgckTu74TZ4p
cCQtQxt+bL4yHh3/CaiFon/OCK2kusAnajjqXCjwp8fmhbve5gI1t9QZXLy2QN9i
4cs6ppl+i/e+Ts0yl0xziwecso7Xcx3UwXbzfNK+UQ1p8UhSLm8xaU0LVu6aNEcz
+E+TevCn
=deRF
-----END PGP SIGNATURE-----

Timestamp of file with hash 3dfd17a565ae7ddbda9eaa8abb03935bc4380ceae2f4f25005f5f9c93ad5a7c8 -

self.log.info('Test that getblock with verbosity 2 includes expected fee')
block = node.getblock(blockhash, 2)
tx = block['tx'][1]
assert 'fee' in tx
Copy link
Member

@MarcoFalke MarcoFalke Dec 24, 2020

Choose a reason for hiding this comment

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

no need for this check. The next line would already fail with KeyError

assert_equal(tx['fee'], tx['vsize'] * fee_per_byte)

self.log.info("Test that getblock with verbosity 2 still works with pruned Undo data")
datadir = get_datadir_path(self.options.tmpdir, 0)
Copy link
Member

@MarcoFalke MarcoFalke Dec 24, 2020

Choose a reason for hiding this comment

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

no need for the import node.datadir should suffice

@MarcoFalke MarcoFalke merged commit f656165 into bitcoin:master Dec 24, 2020
5 checks passed
sidhujag pushed a commit to syscoin/syscoin that referenced this issue Dec 24, 2020
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet