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: faster getblockstats using BlockUndo data #14802

Merged
merged 1 commit into from May 10, 2019

Conversation

Projects
None yet
10 participants
@FelixWeis
Copy link
Contributor

commented Nov 25, 2018

Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for -txindex, and works for all non-pruned blocks.

# 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
seq 550100 550200  0.00s user 0.00s system 62% cpu 0.004 total
xargs -n1 src/bitcoin-cli getblockstats  0.21s user 0.19s system 17% cpu 2.302 total


# 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
seq 550100 550200  0.00s user 0.00s system 87% cpu 0.002 total
xargs -n1 src/bitcoin-cli getblockstats  0.24s user 0.22s system 0% cpu 3:19.42 total
@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2018

<3 I like this direction, certainly having a RPC that requires txindex is unfortunate, as does having rpcs that don't work (well) with pruning. I don't think it would have been worth it to store extra data to make stats fast, but since we already are...

@MarcoFalke
Copy link
Member

left a comment

Concept ACK

@@ -128,8 +113,8 @@ def run_test(self):
assert_equal(stats_by_hash, self.expected_stats[i])

# Check with the node that has no txindex

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 25, 2018

Member

The getblockstats code seems to no longer call into any txindex code (not even as fallback), so might as well remove the txindexed node from the whole test?

This comment has been minimized.

Copy link
@FelixWeis

FelixWeis Feb 9, 2019

Author Contributor

test and testdata changed

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2018

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

Conflicts

No conflicts as of last run.

Show resolved Hide resolved src/validation.cpp Outdated
@promag
Copy link
Member

left a comment

Great change!

Mind adding a release note?

Show resolved Hide resolved src/validation.h Outdated
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Dec 9, 2018

Clever idea!
Concept ACK

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

ConceptACK, code looks good at first glance

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Good to remove reliance on the transaction index.
utACK 602dd54

@FelixWeis FelixWeis force-pushed the FelixWeis:201811_blockstats_undoblock branch from 602dd54 to b9b0992 Feb 6, 2019

@DrahtBot DrahtBot removed the Needs rebase label Feb 6, 2019

@FelixWeis FelixWeis force-pushed the FelixWeis:201811_blockstats_undoblock branch from 1a5c137 to 76fef26 Feb 8, 2019

Show resolved Hide resolved src/validation.h Outdated
@marcinja
Copy link
Contributor

left a comment

utACK 76fef26

Nice work!

Show resolved Hide resolved src/rpc/blockchain.cpp Outdated
Show resolved Hide resolved doc/release-notes.md Outdated
@@ -1987,6 +1982,7 @@ static UniValue getblockstats(const JSONRPCRequest& request)
maxfeerate = std::max(maxfeerate, feerate);
minfeerate = std::min(minfeerate, feerate);
}
tx_n++;

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Mar 19, 2019

Member

looks fragile. (If some paths in the for loop continue, this would be skipped)

@@ -1913,6 +1913,7 @@ static UniValue getblockstats(const JSONRPCRequest& request)
std::vector<std::pair<CAmount, int64_t>> feerate_array;
std::vector<int64_t> txsize_array;

size_t tx_n = 1;
for (const auto& tx : block.vtx) {

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Mar 19, 2019

Member

style-nit:

Suggested change
for (const auto& tx : block.vtx) {
for (size_t i = 0; i < block.vtx.size(); ++i) {
const auto& tx = block.vtx.at(i);
...// after coinbase check
const auto& txundo = blockUndo.vtxundo.at(i + 1);

MarcoFalke added a commit that referenced this pull request Mar 20, 2019

Merge #15623: refactor: Expose UndoReadFromDisk in header
fa11c03 refactor: Expose UndoReadFromDisk in header (MarcoFalke)

Pull request description:

  It is not possible to calculate the fee of a non-mempool transaction in RPCs unless txindex is active or the prevtxs are passed in through the RPC.

  Fix that issue for confirmed txs by exposing `UndoReadFromDisk` in the header file.

  This pull is a requirement for
  * rpc: faster getblockstats using BlockUndo data #14802
  *  Index for BIP 157 block filters #14121
  * my local patches

Tree-SHA512: 859ea5f2dfb4feac612b50faeb0e2b6c07b83f1d983e519d7647a78058d85c0390fd09ec66b460ae7a4c3b273e81b0013ee9f4bb8dfba0c4782ffaa1fa453ea6
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

@FelixWeis Are you still working on this?

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

Don't let this go down unmaintained... please pick this up @FelixWeis

@FelixWeis FelixWeis force-pushed the FelixWeis:201811_blockstats_undoblock branch from 76fef26 to cc72185 Apr 24, 2019

FelixWeis added a commit to FelixWeis/bitcoin that referenced this pull request Apr 24, 2019

@DrahtBot DrahtBot removed the Needs rebase label Apr 24, 2019

Show resolved Hide resolved src/rpc/blockchain.cpp Outdated

@FelixWeis FelixWeis force-pushed the FelixWeis:201811_blockstats_undoblock branch 2 times, most recently from 93a2573 to a565355 Apr 29, 2019

Show resolved Hide resolved src/rpc/blockchain.cpp Outdated

@FelixWeis FelixWeis force-pushed the FelixWeis:201811_blockstats_undoblock branch from a565355 to 293cb0e Apr 29, 2019

@MarcoFalke
Copy link
Member

left a comment

utACK 293cb0e (only some style comments)

Show signature and timestamp

Signature:

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

utACK 293cb0e2ee (only some style comments)
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg0LQv+J5j+966P/KLR0eT22HQJfZGL8PU8dMwq9WEjwhjrLFcccAwMEd4NPGOJ
GaEUbe5/r8V6lJKkXGJcY4Hs874nWKkmrUYVcF9IFGtqDde/75Xf6/cFGQFdMU+Z
9KjKz3Lj1euVeYXCFJ8NNWKhxVwRMSN2+wbqy3/jCnNSjGUc2ob/vrHOXMBA4Peb
gqfRJyUitjmVWB3vM13H0DP4N1gQc3Zd+ILWptZ3KOk147uJZ/YODbTUlbI0JknM
/N8eGu4zbxUR9tMbaG8en3T0otT0/PYEPXy5eBAmjcGC4eHHZLDsbwLaYUkgRCvp
M/WTkBBg5bk+tMbn+hYj4mGp1GCxnm+6D1JuyZsRUOVvu7Zkv87G41Ss+NBq2ozI
SMsIgVJevTh9AeLpD1nPjS2qxWmmbX9WSJGCN/2dRmCi4tTe0dVWsXY8KtJTbVDl
mqL4zWpxGXW0xjimKNDuPw8ONi9BzKKcCT2tn2MA3y4mXbVBeuqkWuMolUw6l3KW
4j4yc522
=/Qza
-----END PGP SIGNATURE-----

Timestamp of file with hash 41e1a1ea262d1060a49ddc699ae7b5453144563ddbb328c375ebb745876632e1 -

Show resolved Hide resolved doc/release-notes-14802.md Outdated
Show resolved Hide resolved src/rpc/blockchain.cpp Outdated
Show resolved Hide resolved src/rpc/blockchain.cpp Outdated
Show resolved Hide resolved src/rpc/blockchain.h Outdated
@MarcoFalke
Copy link
Member

left a comment

more test nits

Show resolved Hide resolved test/functional/rpc_getblockstats.py Outdated
Show resolved Hide resolved test/functional/rpc_getblockstats.py Outdated
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@FelixWeis You don't actually need to rebase. If you address my style-nits, the conflict will solve itself.

@FelixWeis FelixWeis force-pushed the FelixWeis:201811_blockstats_undoblock branch from 293cb0e to e7f0aea May 9, 2019

@DrahtBot DrahtBot removed the Needs rebase label May 9, 2019

@FelixWeis FelixWeis force-pushed the FelixWeis:201811_blockstats_undoblock branch 2 times, most recently from ca7cd73 to 92cd51e May 9, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented May 10, 2019

@FelixWeis FelixWeis force-pushed the FelixWeis:201811_blockstats_undoblock branch from 92cd51e to 8430f86 May 10, 2019

rpc: faster getblockstats using BlockUndo data
Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for -txindex, and works for all non-pruned blocks.

@FelixWeis FelixWeis force-pushed the FelixWeis:201811_blockstats_undoblock branch from 8430f86 to d20d756 May 10, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented May 10, 2019

re-utACK d20d756

Only changes are:

  • Make tests more deterministic
  • Add and use GetUndoChecked
  • Minor stylistic changes
Show signature and timestamp

Signature:

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

re-utACK d20d7567528e216badb8475df298bb3cec008985

Only changes are:
* Make tests more deterministic
* Add and use GetUndoChecked
* Minor stylistic changes
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh/4QwAwLZrUZJqLo1ctjMWO2SQ+ut4rxMDYaMNSIGOxw61u8J6OIUPsVfKBJLF
s2Gi0Q7XwCwlttNVDARBCQFH4R8IhVXLaY//BCfVPamVwXCO3s1x7GDtLZZrjs2c
Rng9VOhCSZ7wXZmgoQmYwj77pQOU2k1B6FXkMylzf/pt+W/jaANCuGtC4k4k8x/l
sM2sH75nWs2URra42exUbmDoIfohOHWfAxcA0Ni31AWkeRYRazWGmsvA93qL6PtP
/wN5rMNi10gCI83/N16ZZ+iq7djqRoCjkbkqSNd2D0NJOmquEIN4W3GGQJ1BciGF
nu0gLQ12ShXCVf167E6s4xGPsKu9hsiUNJN9xursiD1RUhGtUQXdpIRjodL+lDj8
CZhHeUj0whAXKkS+YddhPO385rzqChebvdWZL5stRlUi4TFKzJM+rvsjGNaEKgxO
wjiUfi1L6v07zstlKFLc1pfri9Yws5b4MT6fO9+9dQrtUm1IcS4NeS4YrKKaiI4X
B9R6OHC3
=xoBb
-----END PGP SIGNATURE-----

Timestamp of file with hash 5dc2003a827858f9302ac84ff924bdf15ada47e85278dce1eee93d098c8e8abd -

@MarcoFalke MarcoFalke merged commit d20d756 into bitcoin:master May 10, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request May 10, 2019

Merge #14802: rpc: faster getblockstats using BlockUndo data
d20d756 rpc: faster getblockstats using BlockUndo data (Felix Weis)

Pull request description:

  Using undo data for a block (rev?????.dat) we can retrieve value information about prevouts and calculate the final transaction fee (rate). This approach is about 80x faster, drops the requirement for `-txindex`, and works for all non-pruned blocks.

  ```
  # 2018-11-25T16:36:19Z Bitcoin Core version v0.17.99.0-edc715240-dirty (release build)
  seq 550100 550200  0.00s user 0.00s system 62% cpu 0.004 total
  xargs -n1 src/bitcoin-cli getblockstats  0.21s user 0.19s system 17% cpu 2.302 total

  # 2018-11-25T16:39:17Z Bitcoin Core version v0.17.0 (release build)
  seq 550100 550200  0.00s user 0.00s system 87% cpu 0.002 total
  xargs -n1 src/bitcoin-cli getblockstats  0.24s user 0.22s system 0% cpu 3:19.42 total
  ```

ACKs for commit d20d75:
  MarcoFalke:
    re-utACK d20d756

Tree-SHA512: 5babc3eb8d2fee2cb23dc12f522656b80737a540cbf2b13390a8f388304c46c064cca76f896b46a6e2abae8cc582d28e1ab20dd4bb17ad6142f20630c2d30c54

@LitecoinZ LitecoinZ referenced this pull request May 31, 2019

Open

Backports from upstream #1

44 of 244 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.