Page MenuHomePhabricator

Add Utxo Commitment wrapper around libsecp256k1

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


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

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

rABC Bitcoin ABC
Lint Passed
No 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

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.

136 ↗(On Diff #3036)

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

68 ↗(On Diff #3036)

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

74 ↗(On Diff #3036)


1 ↗(On Diff #3036)


1 ↗(On Diff #3036)


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)


49 ↗(On Diff #3036)


61 ↗(On Diff #3036)


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

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

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

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

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.

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.

27 ↗(On Diff #4047)
29 ↗(On Diff #4047)

Ok. Will do.

27 ↗(On Diff #4047)

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