Page MenuHomePhabricator

Add Utxo Commitment wrapper around libsecp256k1
AbandonedPublic

Authored by jasonbcox on Feb 9 2018, 12:59.

Details

Reviewers
deadalnix
tomtomtom7
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

This adds the CUtxoCommit class that will be used from the
CoinView classes

Depends D1072

Test Plan

Minimal tests included; more tests will be done via RPC

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D1074_3
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 2757
Build 3625: Bitcoin ABC Buildbot (legacy)
Build 3624: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
tomtomtom7 updated this revision to Diff 3012.Feb 28 2018, 22:45

Fix compile error in serialize methods

tomtomtom7 updated this revision to Diff 3025.Mar 2 2018, 14:05

Change combining constructor to add method

tomtomtom7 updated this revision to Diff 3026.Mar 2 2018, 14:07

No changes; just moved some stuff to previous diff for clarity

tomtomtom7 updated this revision to Diff 3036.EditedMar 3 2018, 15:57

Empty update to reflect changes in D1072

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptMar 6 2018, 11:55
deadalnix requested changes to this revision.Apr 10 2018, 23:00

Mostly smaller changes, except the context stuff.

src/CMakeLists.txt
136 ↗(On Diff #3036)

You probably use tabs when space are needed or vice versa.

src/coins.h
68 ↗(On Diff #3036)

Please remove. This is not an error to serialize this.

src/test/CMakeLists.txt
74 ↗(On Diff #3036)

dito

src/test/utxocommit_tests.cpp
1 ↗(On Diff #3036)

2018

src/utxocommit.cpp
1 ↗(On Diff #3036)

2018

11 ↗(On Diff #3036)

Use atomics.

There should already a context in that codebase somewhere. Why isn't it used ?

18 ↗(On Diff #3036)

This isn't thread safe.

34 ↗(On Diff #3036)

wildnewlineappeared

49 ↗(On Diff #3036)

wildnewlineappeared

61 ↗(On Diff #3036)

wildnewlineappeared

83 ↗(On Diff #3036)
if (...) {
    return error(...);
}

// ...
86 ↗(On Diff #3036)

Is this a guarantee that you get stuff sorted by keys ?

src/utxocommit.h
8 ↗(On Diff #3036)

You probably don't need to include that as coin is only used by ref in there.

12 ↗(On Diff #3036)

Separate headers from this software, imported libs and stdlib.

17 ↗(On Diff #3036)
/**
 * A Utxo Commitment
66 ↗(On Diff #3036)

Is there a problem with using unsigned char ?

Also, other secp256k1 structs are using data instead of d in their public API. So maybe we should rename ?

72 ↗(On Diff #3036)

Move after constructor.

This revision now requires changes to proceed.Apr 10 2018, 23:00
tomtomtom7 updated this revision to Diff 3513.Apr 16 2018, 10:11
tomtomtom7 marked 10 inline comments as done.

Thread safe context initialization and various small fixes

tomtomtom7 marked 7 inline comments as done.Apr 16 2018, 10:19
tomtomtom7 added inline comments.
src/utxocommit.cpp
11 ↗(On Diff #3036)

There are already contexts for verify and sign but they are initialized separately. Multiset doesn't need SECP256K!_CONTEXT_VERIFY or _SIGN.

tomtomtom7 updated this revision to Diff 3593.Apr 26 2018, 11:05
tomtomtom7 marked an inline comment as done.

Revert to signed chars for serialization

This is odd but current practice in the code. Moving
to unsigned is an unrelated refactoring.

tomtomtom7 updated this revision to Diff 4045.May 31 2018, 12:03

Rebasing to master and minor fixes from review at Bitcoin XT

  • const correctness for lazy secp256k1 context
  • clear target in secp256k1 add/remove
tomtomtom7 updated this revision to Diff 4047.Jun 1 2018, 08:10

Use CompactSize instead of VARINT

deadalnix added inline comments.Jun 17 2018, 12:34
src/coins.h
28 ↗(On Diff #4047)

Just put 32 bits for flags and 32 bits for height. It's not like we are storing this anywhere, so it's all good. et's not try to be smart.

66 ↗(On Diff #4047)

There should be a new type of serialization here for commitments. Not to disc work in that case because theonly place we serialize this is to store on disk, but that doesn't seems to me to be ideal down the road.

src/utxocommit.cpp
27 ↗(On Diff #4047)

I don't think this is thread safe.

29 ↗(On Diff #4047)

You can inherit from boost::noncopyable and it'll take care of these kind of things for you.

tomtomtom7 added inline comments.Jun 18 2018, 12:59
src/utxocommit.cpp
27 ↗(On Diff #4047)
29 ↗(On Diff #4047)

Ok. Will do.

deadalnix added inline comments.Jun 19 2018, 00:04
src/utxocommit.cpp
27 ↗(On Diff #4047)

Macro scottmeyerstoldyou: Thread safe singleton Provided to you by Scott Meyers

tomtomtom7 updated this revision to Diff 4150.Jun 21 2018, 13:38
  • Use simplified serialization (4b height + 1b iscoinbase)
  • Use boost::noncopyable
jasonbcox requested changes to this revision.Nov 12 2018, 22:11
jasonbcox added a subscriber: jasonbcox.

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.

If this diff is revisited, also land D1495