Page MenuHomePhabricator
Differential D1117

Integrate UtxoCommitment in CCoinView
AbandonedPublic
Actions

Authored by jasonbcox on Feb 22 2018, 11:51.

Details

Reviewers
tomtomtom7
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
teamcity added a subscriber: teamcity.Feb 22 2018, 11:52
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.
teamcity added a comment.Feb 22 2018, 12:12
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.
Harbormaster failed remote builds in B1903: Diff 2942!Feb 22 2018, 12:13
tomtomtom7 updated this revision to Diff 2943.Feb 22 2018, 16:57
Updated to try a more efficient approach with batching the delta



Still very much WIP
teamcity added a comment.Feb 22 2018, 16:59
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.
teamcity added a comment.Feb 22 2018, 17:20
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
tomtomtom7 updated this revision to Diff 3013.Feb 28 2018, 23:46
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
teamcity added a comment.Feb 28 2018, 23:48
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.
Harbormaster completed remote builds in B1954: Diff 3013.Mar 1 2018, 00:09
teamcity added a comment.Mar 1 2018, 00:09
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.
tomtomtom7 updated this revision to Diff 3014.Mar 1 2018, 09:33
Small refactoring to make it more clear
teamcity added a comment.Mar 1 2018, 09:35
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.
teamcity added a comment.Mar 1 2018, 09:56
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.
Harbormaster completed remote builds in B1955: Diff 3014.Mar 1 2018, 09:56
tomtomtom7 updated this revision to Diff 3015.Mar 1 2018, 10:35
Fix some logic error from the previous diff to D1117
Harbormaster completed remote builds in B1956: Diff 3015.Mar 1 2018, 10:35
teamcity added a comment.Mar 1 2018, 10:37
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.
teamcity added a comment.Mar 1 2018, 10:59
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.
Harbormaster completed remote builds in B1956: Diff 3015.Mar 1 2018, 10:59
tomtomtom7 updated this revision to Diff 3105.Mar 6 2018, 11:55
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
teamcity added a comment.Mar 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.
teamcity added a comment.Mar 6 2018, 12:14
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging passed.
Harbormaster completed remote builds in B2014: Diff 3105.Mar 6 2018, 12:14
tomtomtom7 updated this revision to Diff 3108.Mar 6 2018, 14:08
Add first RPC test for UTXO commitment
teamcity added a comment.Mar 6 2018, 14:08
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.
Harbormaster failed remote builds in B2017: Diff 3108!Mar 6 2018, 14:26
teamcity added a comment.Mar 6 2018, 14:26
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.
tomtomtom7 updated this revision to Diff 3121.Mar 7 2018, 15:39
Include third testvector in RPC test
teamcity added a comment.Mar 7 2018, 15:50
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.
teamcity added a comment.Mar 7 2018, 16:10
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging failed.
Harbormaster failed remote builds in B2025: Diff 3121!Mar 7 2018, 16:10
tomtomtom7 updated this revision to Diff 3131.Mar 8 2018, 14:29
Add another RPC test, fix up some comments.



Completes the diff as ready for review
teamcity added a comment.Mar 8 2018, 14:30
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)
tomtomtom7 updated this revision to Diff 3132.Mar 8 2018, 14:37
Minor fix in comments
teamcity added a comment.Mar 8 2018, 14:38
Build Bitcoin-ABC :: Staging :: Bitcoin-ABC Staging started.
teamcity added a comment.Mar 8 2018, 14:58
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
teamcity added a comment.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)
jasonbcox requested changes to this revision.Nov 12 2018, 22:11
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.
Revision Contents
Changeset List
PathSize
src/
31 lines
78 lines
10 lines
rpc/
5 lines
test/
7 lines
5 lines
64 lines
2 lines
4 lines
119 lines
Diff 3014
View Options
src/coins.h
Loading...
View Options
src/coins.cpp
Loading...
View Options
src/init.cpp
Loading...
View Options
src/rpc/blockchain.cpp
Loading...
View Options
src/test/coins_tests.cpp
Loading...
View Options
src/txdb.h
Loading...
View Options
src/txdb.cpp
Loading...
View Options
src/utxocommit.h
Loading...
View Options
src/utxocommit.cpp
Loading...
View Options
src/validation.cpp
Loading...