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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Fix compile error in serialize methods

Change combining constructor to add method

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

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 marked 10 inline comments as done.

Thread safe context initialization and various small fixes

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

Rebasing to master and minor fixes from review at Bitcoin XT

  • const correctness for lazy secp256k1 context
  • clear target in secp256k1 add/remove

Use CompactSize instead of VARINT

src/coins.h
28

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

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

I don't think this is thread safe.

29

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

src/utxocommit.cpp
27

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

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

If this diff is revisited, also land D1495