Add hash_type MUHASH for gettxoutsetinfo #19145

fjahr · 2020-06-02T22:08:14Z

This is another Pr in the series PRs for Coinstatsindex (see overview in #18000). This PR adds the hash_type option muhash to gettxoutsetinfo through which the user can calculate the serialized muhash of the utxo set. This PR does not use the index yet.

DrahtBot · 2020-06-03T01:22:33Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

validation: UTXO snapshot activation #19806 (validation: UTXO snapshot activation by jamesob)

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.

fjahr · 2020-06-04T13:32:23Z

Added another more extensive test, now ready for review.

Sjors · 2020-06-09T12:55:46Z

Concept ACK. In the followup PR that adds a MuHash index, the RPC documentation should point out that this index can be used to dramatically speed up the RPC call when used with muhash. And that muhash and none will be more responsive than the default (or just change the default, and note that as a breaking change to anyone who uses the legacy hash).

fjahr · 2020-06-11T23:08:41Z

@MarcoFalke I would like to hear your thoughts on 2109165. I had it only in the test at first but put it into the framework because I think it could be useful for other tests as well and since it's just a method it does not affect the use of P2PDataStore in other tests. But the data it returns on each UTXO is kind of specific to my test.

maflcko · 2020-06-11T23:22:54Z

Seems fine to put it wherever you want, as long as reviewers are happy. I guess the decision mostly depends on whether this will be used by other tests.

fjahr · 2020-06-29T19:21:16Z

rebased

jonatack · 2021-01-25T10:53:05Z

re-ACK 3506d90 per git diff 2e5617f 3506d90

Thanks for updating.

jonatack · 2021-01-25T13:04:49Z

This might need a release note (even if the note will be updated in planned follow-ups to this).

Sjors · 2021-01-28T13:46:48Z

Muhash at height 668,047: c263d20036cc983510e9b347cb88e947168dd5665ed00b0e6118d07a7704f810

Sjors

Reviewed 3506d90, almost happy...

Muhash at height 668,212: d0192e6604c3dc9a78f47bfc5ae84b8c0f6baf960d7ba9305664bbb9f65ef092.

hash_serialized_2 is 63fa057a656168ed832d8abbec8008948fc17aecde9f346d69043871a746941f on master, but 15d3629db2fcbed27a1fd9d35c15b5ac6f0a70f5723ebf9cf87e3df0b0feb061 here (efe73b1f89e82d65100c73c06d9a01ec19cc7e029d93caaa8bfded4d1723d484 if I revert GetHash())

You may want to hardcode a hash_serialized_2 example in the test suite.

src/node/coinstats.cpp

src/rpc/blockchain.cpp

src/node/coinstats.cpp

Also small style fix in rpc/util.cpp

fjahr · 2021-01-31T00:14:11Z

Reviewed 3506d90, almost happy...

Muhash at height 668,212: d0192e6604c3dc9a78f47bfc5ae84b8c0f6baf960d7ba9305664bbb9f65ef092.

hash_serialized_2 is 63fa057a656168ed832d8abbec8008948fc17aecde9f346d69043871a746941f on master, but 15d3629db2fcbed27a1fd9d35c15b5ac6f0a70f5723ebf9cf87e3df0b0feb061 here (efe73b1f89e82d65100c73c06d9a01ec19cc7e029d93caaa8bfded4d1723d484 if I revert GetHash())

You may want to hardcode a hash_serialized_2 example in the test suite.

Thanks a lot for testing @Sjors ! I should have addressed all your comments.

The difference in that hash was caused by using outputs.end() where I should have used std::prev(outputs.end()) in ApplyHash(). I was actually sure we had some deterministic tests in place for that hash already but obviously, we didn't. So I added a new commit with a small test calculating the hash based on the cached chain to add such coverage.

jonatack

Good catch and nice improvements.

Tested re-ACK e987ae5 per git diff 3506d90 e987ae5, reviewed diff, debug built, ran gettxoutsetinfo -signet and help on this branch vs master, at height 23127 both returned hash_serialized_2 of 2b72d65f3b6efb2311f58374ea2b939abf49684d44f4bafda45faa3b5452a454 and this branch returned muhash of c9f1ff12d345ccf9939c6bbf087e6f7399b6115adee1569287e9c5c43dbb475c

Only if you have to retouch and if you agree, one nit below (and maybe use single/double quotes consistently in the functional tests).

jonatack · 2021-02-01T20:21:21Z

src/rpc/blockchain.cpp

@@ -1063,7 +1076,8 @@ static RPCHelpMan gettxoutsetinfo()
                        {RPCResult::Type::NUM, "transactions", "The number of transactions with unspent outputs"},
                        {RPCResult::Type::NUM, "txouts", "The number of unspent transaction outputs"},
                        {RPCResult::Type::NUM, "bogosize", "A meaningless metric for UTXO set size"},
-                        {RPCResult::Type::STR_HEX, "hash_serialized_2", "The serialized hash (only present if 'hash_serialized_2' hash_type is chosen)"},
+                        {RPCResult::Type::STR_HEX, "hash_serialized_2", /* optional */ true, "The serialized hash (only present if 'hash_serialized_2' hash_type is chosen)"},
+                        {RPCResult::Type::STR_HEX, "muhash", /* optional */ true, "The serialized hash (only present if 'muhash' hash_type is chosen)"},


nit, in these two lines, could replace the single quotes with double ones for consistency, e.g. s/'/\"/

Yes, will do if I have to retouch! Thanks for re-reviewing!

Sjors · 2021-02-03T10:19:24Z

tACK e987ae5

I get the same MuHash as before for block 668,212 and hash_serialized_2 now matches what I found on master.

achow101 · 2021-02-08T22:18:18Z

ACK e987ae5

It would be nice if the test used MiniWallet or otherwise didn't rely on the wallet, but it's fine for now.

ryanofsky

Code review ACK e987ae5. Looks very good. I left one suggestion to simplify code, but feel free to ignore it here and maybe consider it for later since PR has already had a lot of review.

ryanofsky · 2021-02-10T17:59:26Z

src/crypto/muhash.cpp

-    uint256 hashed_in = (CHashWriter(SER_DISK, 0) << in).GetSHA256();
-    unsigned char tmp[BYTE_SIZE];
-    ChaCha20(hashed_in.data(), hashed_in.size()).Keystream(tmp, BYTE_SIZE);
+Num3072::Num3072(const unsigned char (&data)[BYTE_SIZE]) {


In commit "refactor: Improve encapsulation between MuHash3072 and Num3072" (a1fccea)

Note in case it helps other reviewers: I found this commit easier to review rearranging the method order to ToNum3072 Num3072 ToBytes instead of Num3072 ToBytes ToNum3072 so diff was smaller with code moving around less.

ryanofsky · 2021-02-10T18:32:02Z

src/node/coinstats.cpp

@@ -24,31 +24,35 @@ static uint64_t GetBogoSize(const CScript& scriptPubKey)
           scriptPubKey.size() /* scriptPubKey */;
 }

-static void ApplyStats(CCoinsStats& stats, CHashWriter& ss, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
+static void ApplyHash(CCoinsStats& stats, CHashWriter& ss, const uint256& hash, const std::map<uint32_t, Coin>& outputs, std::map<uint32_t, Coin>::const_iterator it)


In commit "refactor: Separate hash and stats calculation in coinstats" (2474645)

stats arguments in unused in all three overloads and could be dropped.

Also, passing iterators around like this seems unnecessarily elaborate. It makes sense to separate stats and hash computations, but is there a reason both computations need to share the a single for loop over the output map? I would think you could make all the functions separate like:

static void ApplyHash(CHashWriter& ss, const uint256& hash, const std::map<uint32_t, Coin>& outputs);
static void ApplyHash(MuHash3072& muhash, const uint256& hash, const std::map<uint32_t, Coin>& outputs);
static void ApplyHash(std::nullptr_t, const uint256& hash, const std::map<uint32_t, Coin>& outputs);
static void ApplyStats(CCoinsStats& stats, const uint256& hash, const std::map<uint32_t, Coin>& outputs);

fjahr · 2021-02-10T21:43:45Z

Thanks a lot for the reviews @achow101 and @ryanofsky ! I will add your suggestions if I have to push again, otherwise I will put them into the next PR in this series.

e987ae5 test: Add test for deterministic UTXO set hash results (Fabian Jahr) 6ccc8fc test: Add test for gettxoutsetinfo RPC with MuHash (Fabian Jahr) 0d3b2f6 rpc: Add hash_type MUHASH to gettxoutsetinfo (Fabian Jahr) 2474645 refactor: Separate hash and stats calculation in coinstats (Fabian Jahr) a1fccea refactor: Improve encapsulation between MuHash3072 and Num3072 (Fabian Jahr) Pull request description: This is another Pr in the series PRs for Coinstatsindex (see overview in bitcoin#18000). This PR adds the `hash_type` option `muhash` to `gettxoutsetinfo` through which the user can calculate the serialized muhash of the utxo set. This PR does not use the index yet. ACKs for top commit: Sjors: tACK e987ae5 achow101: ACK e987ae5 jonatack: Tested re-ACK e987ae5 per `git diff 3506d90 e987ae5`, reviewed diff, debug built, ran gettxoutsetinfo -signet and help on this branch vs master, at height 23127 both returned `hash_serialized_2` of `2b72d65f3b6efb2311f58374ea2b939abf49684d44f4bafda45faa3b5452a454` and this branch returned `muhash` of `c9f1ff12d345ccf9939c6bbf087e6f7399b6115adee1569287e9c5c43dbb475c` ryanofsky: Code review ACK e987ae5. Looks very good. I left one suggestion to simplify code, but feel free to ignore it here and maybe consider it for later since PR has already had a lot of review. Tree-SHA512: 9a739ce375e73749fa69a467262b60d3e5314ef384e2d7150b3bbc8e4125cd9fd1db95306623bb9a632fcbaf5d9d2bf2f5cc43bf717d4ff5e2c9c4b52dd9296c

maflcko · 2021-02-12T16:12:00Z

test/functional/feature_utxo_set_hash.py

+    def test_deterministic_hash_results(self):
+        self.log.info("Test deterministic UTXO set hash results")
+
+        # These depend on the setup_clean_chain option, the chain loaded from the cache


there is no loaded chain and the utxo set is empty. Which is also useful to check, but doesn't check if the hash is deterministic?

Did you forget to load the chain from ./test/functional/data/rpc_getblockstats.json?

4f2653a test: Use deterministic chain in utxo set hash test (Fabian Jahr) 4973c51 test: Remove wallet dependency of utxo set hash test (Fabian Jahr) 1a27af1 rpc: Improve gettxoutsetinfo help (Fabian Jahr) Pull request description: Follow-ups to bitcoin#19145: - Small improvement on the help text of RPC gettxoutsetinfo - Using deterministic blockchain in the test `functional/feature_utxo_set_hash.py` - Removing wallet dependency in the test `functional/feature_utxo_set_hash.py` Split out of bitcoin#19521. ACKs for top commit: MarcoFalke: review ACK 4f2653a 👲 Tree-SHA512: 92927b3aa22b6324eb4fc9d346755313dec44d973aa69a0ebf80a8569b5f3a7cf3539721ebdba183737534b9e29b3e33f412515890f0d0b819878032a3bba8f9

Summary: This is a backport of [[bitcoin/bitcoin#19145 | core#19145]] [1/3] bitcoin/bitcoin@a1fccea Depends on D11150 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D11155

Summary: This is a backport of [[bitcoin/bitcoin#19145 | core#19145]] [2/3] bitcoin/bitcoin@2474645 Depends on D11155 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11156

Summary: This concludes backport of [[bitcoin/bitcoin#19145 | core#19145]] [3/3] bitcoin/bitcoin@0d3b2f6 bitcoin/bitcoin@6ccc8fc bitcoin/bitcoin@e987ae5 Depends on D11162 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D11157

fjahr mentioned this pull request Jun 2, 2020

[WIP] Index for UTXO Set Statistics #18000

Closed

fjahr changed the title ~~[WIP] Allow hash_type options for gettxoutsetinfo~~ [WIP] Add hash_type options for gettxoutsetinfo Jun 2, 2020

DrahtBot added Build system RPC/REST/ZMQ Tests Utils/log/libs labels Jun 2, 2020

DrahtBot mentioned this pull request Jun 3, 2020

Make SER_GETHASH implicit for CHashWriter and SerializeHash #13462

Closed

fjahr changed the title ~~[WIP] Add hash_type options for gettxoutsetinfo~~ Add hash_type options for gettxoutsetinfo Jun 4, 2020

fjahr mentioned this pull request Jun 5, 2020

Add MuHash3072 implementation #19055

Merged

This was referenced Jun 6, 2020

Add Muhash3072 implementation in Python #19105

Merged

Add ASM optimizations for MuHash3072 #19181

Closed

fjahr force-pushed the csi-3-muhash-rpc branch from 6013f04 to 7841f05 Compare June 11, 2020 23:02

maflcko removed Tests Build system labels Jun 11, 2020

DrahtBot mentioned this pull request Jun 12, 2020

test: move sync_blocks and sync_mempool functions to test_framework.py #19208

Merged

fjahr force-pushed the csi-3-muhash-rpc branch from 7841f05 to 8d1216d Compare June 12, 2020 12:57

fjahr mentioned this pull request Jun 19, 2020

Add gettxoutsetinfo hash_type option #19328

Merged

DrahtBot added the Needs rebase label Jun 24, 2020

fjahr force-pushed the csi-3-muhash-rpc branch from 8d1216d to 73a0a0d Compare June 29, 2020 19:20

DrahtBot removed the Needs rebase label Jun 29, 2020

Sjors suggested changes Jan 29, 2021

View reviewed changes

src/node/coinstats.cpp Show resolved Hide resolved

src/node/coinstats.cpp Show resolved Hide resolved

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

src/node/coinstats.cpp Outdated Show resolved Hide resolved

fjahr added 4 commits January 30, 2021 16:51

refactor: Separate hash and stats calculation in coinstats

2474645

rpc: Add hash_type MUHASH to gettxoutsetinfo

0d3b2f6

Also small style fix in rpc/util.cpp

test: Add test for gettxoutsetinfo RPC with MuHash

6ccc8fc

test: Add test for deterministic UTXO set hash results

e987ae5

fjahr force-pushed the csi-3-muhash-rpc branch from 3506d90 to e987ae5 Compare January 31, 2021 00:00

jonatack reviewed Feb 1, 2021

View reviewed changes

ryanofsky approved these changes Feb 10, 2021

View reviewed changes

laanwj merged commit 8d82edd into bitcoin:master Feb 12, 2021

maflcko reviewed Feb 12, 2021

View reviewed changes

fjahr mentioned this pull request Mar 8, 2021

test: Test improvements for UTXO set hash tests #21390

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Feb 26, 2022

merge bitcoin#19145: Add hash_type MUHASH for gettxoutsetinfo

c46d0b9

kwvg added a commit to kwvg/dash that referenced this pull request Feb 26, 2022

merge bitcoin#19145: Add hash_type MUHASH for gettxoutsetinfo

47e6598

kwvg added a commit to kwvg/dash that referenced this pull request Apr 3, 2022

merge bitcoin#19145: Add hash_type MUHASH for gettxoutsetinfo

88381c4

kwvg added a commit to kwvg/dash that referenced this pull request Apr 20, 2022

merge bitcoin#19145: Add hash_type MUHASH for gettxoutsetinfo

f559701

kwvg added a commit to kwvg/dash that referenced this pull request Apr 24, 2022

merge bitcoin#19145: Add hash_type MUHASH for gettxoutsetinfo

6504caf

kwvg added a commit to kwvg/dash that referenced this pull request Apr 27, 2022

merge bitcoin#19145: Add hash_type MUHASH for gettxoutsetinfo

ef18d08

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hash_type MUHASH for gettxoutsetinfo #19145

Add hash_type MUHASH for gettxoutsetinfo #19145

fjahr commented Jun 2, 2020 •

edited by jnewbery

Loading

DrahtBot commented Jun 3, 2020 •

edited

Loading

fjahr commented Jun 4, 2020

Sjors commented Jun 9, 2020 •

edited

Loading

fjahr commented Jun 11, 2020

maflcko commented Jun 11, 2020

fjahr commented Jun 29, 2020

jonatack commented Jan 25, 2021

jonatack commented Jan 25, 2021

Sjors commented Jan 28, 2021

Sjors left a comment •

edited

Loading

fjahr commented Jan 31, 2021

jonatack left a comment

jonatack Feb 1, 2021

fjahr Feb 1, 2021

Sjors commented Feb 3, 2021

achow101 commented Feb 8, 2021

ryanofsky left a comment

ryanofsky Feb 10, 2021

ryanofsky Feb 10, 2021

fjahr commented Feb 10, 2021

maflcko Feb 12, 2021

Add hash_type MUHASH for gettxoutsetinfo #19145

Add hash_type MUHASH for gettxoutsetinfo #19145

Conversation

fjahr commented Jun 2, 2020 • edited by jnewbery Loading

DrahtBot commented Jun 3, 2020 • edited Loading

Conflicts

fjahr commented Jun 4, 2020

Sjors commented Jun 9, 2020 • edited Loading

fjahr commented Jun 11, 2020

maflcko commented Jun 11, 2020

fjahr commented Jun 29, 2020

jonatack commented Jan 25, 2021

jonatack commented Jan 25, 2021

Sjors commented Jan 28, 2021

Sjors left a comment • edited Loading

Choose a reason for hiding this comment

fjahr commented Jan 31, 2021

jonatack left a comment

Choose a reason for hiding this comment

jonatack Feb 1, 2021

Choose a reason for hiding this comment

fjahr Feb 1, 2021

Choose a reason for hiding this comment

Sjors commented Feb 3, 2021

achow101 commented Feb 8, 2021

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Feb 10, 2021

Choose a reason for hiding this comment

ryanofsky Feb 10, 2021

Choose a reason for hiding this comment

fjahr commented Feb 10, 2021

maflcko Feb 12, 2021

Choose a reason for hiding this comment

fjahr commented Jun 2, 2020 •

edited by jnewbery

Loading

DrahtBot commented Jun 3, 2020 •

edited

Loading

Sjors commented Jun 9, 2020 •

edited

Loading

Sjors left a comment •

edited

Loading