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 Passed
Unit
No Test Coverage
Build Status
Buildable 2031
Build 2210: Bitcoin ABC Buildbot (legacy)
Build 2209: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Feb 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)

Updated to try a more efficient approach with batching the delta

Still very much WIP

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

Small refactoring to make it more clear

Fix some logic error from the previous diff to D1117

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

Add first RPC test for UTXO commitment

Include third testvector in RPC test

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)
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 inline comments.
src/coins.cpp
328 ↗(On Diff #3132)

Probably best to log an error for the specific case that you described.

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.

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 abandoned this revision.
jasonbcox edited reviewers, added: tomtomtom7; removed: jasonbcox.