This adds the CUtxoCommit class that will be used from the
CoinView classes
Depends D1072
Details
Details
- Reviewers
deadalnix tomtomtom7 - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project
Minimal tests included; more tests will be done via RPC
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- utxo1
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 1815 Build 1823: Bitcoin ABC Buildbot (legacy) Build 1822: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
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) |
|
| 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 ? |
| 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. |
| 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. |
Comment Actions
Revert to signed chars for serialization
This is odd but current practice in the code. Moving
to unsigned is an unrelated refactoring.
Comment Actions
Rebasing to master and minor fixes from review at Bitcoin XT
- const correctness for lazy secp256k1 context
- clear target in secp256k1 add/remove
| 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. |
| src/utxocommit.cpp | ||
|---|---|---|
| 27 ↗ | (On Diff #4047) | I'm pretty sure it is since C+11. http://www.modernescpp.com/index.php/thread-safe-initialization-of-a-singleton |
| 29 ↗ | (On Diff #4047) | Ok. Will do. |
| src/utxocommit.cpp | ||
|---|---|---|
| 27 ↗ | (On Diff #4047) |
|
Comment Actions
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.