Details

Reviewers

Group Reviewers

Restricted Owners Package	(Owns No Changed Paths)
Restricted Project

Summary

Integrates maintaining the UTXO Commitment in the CCoinView classes and LevelDB.

Exposes the maintained commitment in gettxoutsetinfo as "commitment" and exposes a calculated on-the-fly commitment as "commitment_calculated"

Depends on D1074

Test Plan

Two RPC tests included

Diff Detail

Repository

rABC Bitcoin ABC

Branch

utxof

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 2032
Build 2212: Bitcoin ABC Buildbot (legacy)
Build 2211: arc lint + arc unit

Event Timeline

tomtomtom7 created this revision.Feb 22 2018, 11:51

Owners added a reviewer: Restricted Owners Package.Feb 22 2018, 11:51

Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 22 2018, 11:51

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

tomtomtom7 retitled this revision from Work-in-Progress First attempt at integrating the commitment CCoinViews the easy way. This does not yet update the commitment in CCoinViewDB, only in CCoinViewCache to test IBD performance. It's to slow so I will be updating this to try... to WIP Integrate UtxoCommitment in CCoinView.Feb 22 2018, 11:53

tomtomtom7 edited the summary of this revision. (Show Details)

tomtomtom7 added a parent revision: D1110: Add UTXO commitment contruction from UTXO cursor.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.

Harbormaster failed remote builds in B1903: Diff 2942!Feb 22 2018, 12:13

Updated to try a more efficient approach with batching the delta

Still very much WIP

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B1904: Diff 2943.Feb 22 2018, 17:20

tomtomtom7 mentioned this in D1074: Add Utxo Commitment wrapper around libsecp256k1.Feb 26 2018, 19:01

Still WIP, But much better version.

This holds back maintaining the UTXO commitment during IBD,
and picks it up after.

It also adds a field to the gettxoutsetinfo rpc

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Harbormaster completed remote builds in B1954: Diff 3013.Mar 1 2018, 00:09

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Small refactoring to make it more clear

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B1955: Diff 3014.Mar 1 2018, 09:56

Fix some logic error from the previous diff to D1117

Harbormaster completed remote builds in B1956: Diff 3015.Mar 1 2018, 10:35

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B1956: Diff 3015.Mar 1 2018, 10:59

Fix several logic errors and add commitment_calculated to the gettxoutsetinfo

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMar 6 2018, 11:55

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

tomtomtom7 edited the summary of this revision. (Show Details)Mar 6 2018, 11:55

tomtomtom7 added a parent revision: D1074: Add Utxo Commitment wrapper around libsecp256k1.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2014: Diff 3105.Mar 6 2018, 12:14

Add first RPC test for UTXO commitment

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Harbormaster failed remote builds in B2017: Diff 3108!Mar 6 2018, 14:26

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.

Include third testvector in RPC test

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.

Harbormaster failed remote builds in B2025: Diff 3121!Mar 7 2018, 16:10

Add another RPC test, fix up some comments.

Completes the diff as ready for review

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

tomtomtom7 retitled this revision from WIP Integrate UtxoCommitment in CCoinView to Integrate UtxoCommitment in CCoinView.Mar 8 2018, 14:34

tomtomtom7 edited the summary of this revision. (Show Details)

tomtomtom7 edited the test plan for this revision. (Show Details)

Minor fix in comments

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

Harbormaster completed remote builds in B2031: Diff 3131.Mar 8 2018, 14:58

Harbormaster completed remote builds in B2032: Diff 3132.Mar 8 2018, 15:06

Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.

tomtomtom7 mentioned this in T290: Maintain UTXO commitment and expose RPC.Mar 15 2018, 13:00

tomtomtom7 added a child revision: D1474: Include unverified UTXO commitment in Coinbase on generate.May 31 2018, 14:41

ivan_s added a subscriber: ivan_s.Jun 14 2018, 14:51

ivan_s added inline comments.

src/coins.cpp
328	If it's ever the case that `parent != nullptr` but `cacheUtxoCommitDelta == nullptr`, there might be a memory leak. Also the caller of this method should not have to perform a `delete`. I think it would be better to work with a `unique_ptr` in this case.

jasonbcox added a subscriber: jasonbcox.Jun 14 2018, 15:01

jasonbcox added inline comments.

src/coins.cpp
328	Probably best to log an error for the specific case that you described.

tomtomtom7 added inline comments.Jun 15 2018, 08:35

src/coins.cpp
328	Yes. I believe this is possible due to the way assumevalid works. Initially the headers aren't in yet, so blocks aren't assumed valid and the commitment is maintained. Once assumevalid kicks in, the commitment is cleared.
328	Yes. Good point about unique_ptr. Will change.

jasonbcox added inline comments.Jun 27 2018, 20:23

src/validation.cpp
2661	mainting -> maintaining
test/functional/utxocommit.py
7	remove the empty comment line
30	Is this a comment? seems weird to have a string floating in the middle of the test like this
68	Appending the "0x" should be unnecessary. You can use `int(self.nodes[0].getbestblockhash(), 16)` for tip instead.
108	This comment is describing what (which I can get from the code) instead of why (which is not obvious at first glance)

Trying to clear off the review queue for a bit since this is quite old. Please rebase this on master when you want a review again.

This revision now requires changes to proceed.Nov 12 2018, 22:11

jasonbcox commandeered this revision.Apr 23 2019, 17:01

jasonbcox abandoned this revision.

jasonbcox edited reviewers, added: tomtomtom7; removed: jasonbcox.

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	2942	23cc558		Feb 22 2018, 11:51	★	★
Diff 2	2943	23cc558	Updated to try a more efficient approach with batching the delta	Feb 22 2018, 16:57	★	★
Diff 3	3013	532fc18	Still WIP, But much better version.	Feb 28 2018, 23:46	★	★
Diff 4	3014	532fc18	Small refactoring to make it more clear	Mar 1 2018, 09:33	★	★
Diff 5	3015	532fc18	Fix some logic error from the previous diff to D1117	Mar 1 2018, 10:35	★	★
Diff 6	3105	6176133	Fix several logic errors and add commitment_calculated to the gettxoutsetinfo	Mar 6 2018, 11:55	★	★
Diff 7	3108	6176133	Add first RPC test for UTXO commitment	Mar 6 2018, 14:08	★	★
Diff 8	3121	475a2dd	Include third testvector in RPC test	Mar 7 2018, 15:39	★	★
Diff 9	3131	475a2dd	Add another RPC test, fix up some comments.	Mar 8 2018, 14:28	★	★
Diff 10	3132	475a2dd	Minor fix in comments	Mar 8 2018, 14:36	★	★

Commit	Tree	Parents	Author	Summary	Date
fdf6ecb1fbb3	fb4a8372d874	475a2dd27fdf	Tomas van der Wansem	Integrate UtxoCommitment in CCoinView (Show More…)	Mar 5 2018, 20:17

Status	Author	Revision
Abandoned	jasonbcox	D1495 Add unit tests for combining UTXO commitments.
Abandoned	jasonbcox	D1474 Include unverified UTXO commitment in Coinbase on generate
Abandoned	jasonbcox	D1117 Integrate UtxoCommitment in CCoinView
Abandoned	tomtomtom7	D1110 Add UTXO commitment contruction from UTXO cursor
Abandoned	jasonbcox	D1074 Add Utxo Commitment wrapper around libsecp256k1
Closed	tomtomtom7	D1072 Add ECMH multiset module to libsecp256k1

D3894 Merge #12737: Remove unnecessary NONNEGATIVE_SIGNED

·Reviewers: deadalnix, Fabien, jasonbcox, Restricted Owners Package, Restricted Project

Sat, Nov 23, 14:59

Author: nakihito

		Path
M		src/coins.h (32 lines)
M		src/coins.cpp (93 lines)
M		src/init.cpp (10 lines)
M		src/rpc/blockchain.cpp (14 lines)
M		src/test/coins_tests.cpp (5 lines)
M		src/txdb.h (5 lines)
M		src/txdb.cpp (48 lines)
M		src/utxocommit.h (2 lines)
M		src/utxocommit.cpp (4 lines)
M		src/validation.cpp (127 lines)
M		test/functional/test_runner.py (2 lines)
A	M	test/functional/utxocommit.py (201 lines)
A	M	test/functional/utxocommit_vector.py (100 lines)

Integrate UtxoCommitment in CCoinView
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 3132

src/coins.h

src/coins.cpp

src/init.cpp

src/rpc/blockchain.cpp

src/test/coins_tests.cpp

src/txdb.h

src/txdb.cpp

src/utxocommit.h

src/utxocommit.cpp

src/validation.cpp

test/functional/test_runner.py

test/functional/utxocommit.py

test/functional/utxocommit_vector.py

Integrate UtxoCommitment in CCoinViewAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 3132

src/coins.h

src/coins.cpp

src/init.cpp

src/rpc/blockchain.cpp

src/test/coins_tests.cpp

src/txdb.h

src/txdb.cpp

src/utxocommit.h

src/utxocommit.cpp

src/validation.cpp

test/functional/test_runner.py

test/functional/utxocommit.py

test/functional/utxocommit_vector.py

Integrate UtxoCommitment in CCoinView
AbandonedPublic
Actions

Revision Contents
Changeset List