Page MenuHomePhabricator

Integrate UtxoCommitment in CCoinView
AbandonedPublic

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
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
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 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

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

tomtomtom7 updated this revision to Diff 3014.Mar 1 2018, 09:33

Small refactoring to make it more clear

tomtomtom7 updated this revision to Diff 3015.Mar 1 2018, 10:35

Fix some logic error from the previous diff to D1117

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
tomtomtom7 updated this revision to Diff 3108.Mar 6 2018, 14:08

Add first RPC test for UTXO commitment

tomtomtom7 updated this revision to Diff 3121.Mar 7 2018, 15:39

Include third testvector in RPC test

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

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

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 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)

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.