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

utxo4

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 1955
Build 2060: Bitcoin ABC Buildbot (legacy)
Build 2059: 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 ↗	(On Diff #3132)	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 ↗	(On Diff #3132)	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 ↗	(On Diff #3132)	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 ↗	(On Diff #3132)	Yes. Good point about unique_ptr. Will change.

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

src/validation.cpp
2661 ↗	(On Diff #3132)	mainting -> maintaining
test/functional/utxocommit.py
7 ↗	(On Diff #3132)	remove the empty comment line
30 ↗	(On Diff #3132)	Is this a comment? seems weird to have a string floating in the middle of the test like this
68 ↗	(On Diff #3132)	Appending the "0x" should be unnecessary. You can use `int(self.nodes[0].getbestblockhash(), 16)` for tip instead.
108 ↗	(On Diff #3132)	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.

Integrate UtxoCommitment in CCoinView
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 3014

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

Integrate UtxoCommitment in CCoinViewAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 3014

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

Integrate UtxoCommitment in CCoinView
AbandonedPublic
Actions

Revision Contents
Changeset List