First part of DIP3 implementation by codablock · Pull Request #2246 · dashpay/dash

codablock · 2018-08-30T05:51:26Z

This is extracted from #2083. It only contains ProRegTx, the deterministic masternode manager and RPCs for ProRegTx.

With this PR, it's possible to create, validate and mine ProRegTx on-chain. The commits in this PR have the refactorings included which happened later in #2083 squashed-in.

@UdjinM6 Please merge in non-squashed mode again :)

Allows easier commit/rollback handling, especially useful when AcceptBlock fails and things need to be reverted.

UdjinM6

See inline comments

UdjinM6 · 2018-08-30T12:48:26Z

proTx.nProtocolVersion > PROTOCOL_VERSION will cause old nodes to reject new MNs during every migration with protobump (and fork off). I doubt it's a good idea tbh.

Ah, forgot to apply a change that happened later in #2083. Will do in a few minutes

It was de51377 and I squashed that into this PR now

UdjinM6 · 2018-08-30T12:53:01Z

This is going to break IBD/reindex if/when we are going to bump CProRegTx::CURRENT_VERSION. ptx.nVersion > CProRegTx::CURRENT_VERSION would solve this for future versions but old nodes are still going to suffer and will reject MNs with new CProRegTx::CURRENT_VERSION (and fork off)... is this really what we want?

Whenever we bump CURRENT_VERSION, we'll have to use some kind of deployment logic...either BIP9 or a spork to activate it. So, new versions would only be allowed after it's ensured most of the network has upgraded. And at the same time, we'd modify this if to allow both versions (or only the new one after activation).

I fear that allowing ptx.nVersion >= CProRegTx::CURRENT_VERSION would allow people to put whatever version they want on-chain, making it later hard to do real version bumps.

By ptx.nVersion > CProRegTx::CURRENT_VERSION I meant that we would reject any protx with version above currently known one while still accepting/processing the old ones. The logic for different/additional checks for new version protxes would then follow this one.

Ah now I understand. I'll change it to ptx.nVersion > CProRegTx::CURRENT_VERSION then. In the case of bumping the version, the thing with the deployment logic applies. Or do I still miss something?

No, that's it - adding this fix is all I wanted for now (assuming that deployment logic for new version is smth to be added whenever we decide to actually bump it) :)

UdjinM6 · 2018-08-30T12:55:51Z

should probably be uint16_t too?

UdjinM6 · 2018-08-30T12:57:02Z

70210 and pre-70210 (mainnet) nodes are not able to recognise special txes, so 70211 maybe?

UdjinM6 · 2018-08-30T13:07:29Z

should be removed

codablock · 2018-08-30T14:56:18Z

Applied review fixes (squashed into original commits)

UdjinM6

see inline comments

UdjinM6 · 2018-08-30T15:06:19Z

should be removed

UdjinM6 · 2018-08-30T15:08:24Z

let's follow naming convention for all the ints above

UdjinM6 · 2018-08-30T15:09:08Z

UdjinM6 · 2018-08-30T15:09:53Z

UdjinM6 · 2018-08-30T15:10:29Z

UdjinM6 · 2018-08-30T15:14:32Z

Let's maybe move non-oneliners/non-serialization/non-op implementations to cpp?

Done, except for template functiond and the boost::adaptors stuff in CDeterministicMNList as I want to make sure these get inlined.

UdjinM6 · 2018-08-30T15:14:47Z

UdjinM6 · 2018-08-30T15:26:13Z

Hmmm... This is not what it seems to be at the first sight. I expected this to be a (max) number of cached entries but from CDeterministicMNManager::CleanupCache() code it seems that it's not the case and LISTS_CACHE_SIZE is actually a number of blocks to keep the cache. That's confusing imo. Why don't we actually define a size of a cache and store up to N recent entries instead of trying to figure out LISTS_CACHE_SIZE vs SNAPSHOT_LIST_PERIOD relationship to get the idea how many cache entries we are going to store?

This was my initial plan, but then I realized that someone might be able to come up with a way to rotate the cache multiple times in a single block, causing GetListForBlock to not use any cached entries and thus slow down block processing. Not sure if this would be really possible as of now, but if we ever introduce some special transaction that accesses old lists this might become an attack vector. So I preferred to use a caching mechanism that did a cleanup after block processing.

Just checked the implementation of CacheMap from cachemap.h. If we add a boolean template parameter to this map that disables automatic pruning and instead allows manual pruning, we could use that map implementation for this kind of "cleanup" behavior. What do you think?

Ok, the argument about organising cache this way makes sense, I agree.

UdjinM6 · 2018-08-30T15:45:05Z

this is actually a pointer

state is already used as an instance of CValidationState all over the place

Let's maybe rename this to pdmnState or smth like that?

Same (confusing naming i.e. no p prefix) applies to a bunch of other *CPtrs actually but this one is the most confusing one imo.

I'll change this one for now but leave the other ones as they are. The "dmn" is used in all commits from #2083 and it will be a hell to fix this in all commits.

schinzelh · 2018-08-30T17:26:18Z

nit: you should update your template to 2018 :)

Also add transaction handling to ConnectTip and DisconnectTip and a few other places where blocks are processed.

Avoid accidential spending of ProTx collaterals.

codablock · 2018-08-30T18:03:43Z

Pushed more review fixes, including the naming convention fixes. All as squashed changed.

I omitted the caching change for now and will add this later after we're done with the discussion.

UdjinM6

utACK

UdjinM6 · 2018-08-30T18:59:31Z

Ok, the argument about organising cache this way makes sense, I agree.

… register_*` to be known by the wallet This requirement was introduced in the initial implementation when we were signing protx payload with the owner key (dashpay#2246). This was changed later when we implemented external collateral references (dashpay#2366). The owner key is not used for anything in ProTxReg but to get CKeyID since then which we can do by simply decoding an address instead. This simplifies masternode registration process by letting an Operator to issue `protx register_prepare` on its own instead of asking an Owner. An Owner still have to sign the message provided by an Operator with his collateral address to prove collateral ownership (and authorize masternode registration). Note: `protx register_*` rpc will no longer accept privkeys for ownerAddress. Some 3-rd party software like DMT might need to be patched to work correctly with nodes running with this fix.

… register_*` to be known by the wallet (#3773) This requirement was introduced in the initial implementation when we were signing protx payload with the owner key (#2246). This was changed later when we implemented external collateral references (#2366). The owner key is not used for anything in ProTxReg but to get CKeyID since then which we can do by simply decoding an address instead. This simplifies masternode registration process by letting an Operator to issue `protx register_prepare` on its own instead of asking an Owner. An Owner still have to sign the message provided by an Operator with his collateral address to prove collateral ownership (and authorize masternode registration). Note: `protx register_*` rpc will no longer accept privkeys for ownerAddress. Some 3-rd party software like DMT might need to be patched to work correctly with nodes running with this fix.

… register_*` to be known by the wallet This requirement was introduced in the initial implementation when we were signing protx payload with the owner key (dashpay#2246). This was changed later when we implemented external collateral references (dashpay#2366). The owner key is not used for anything in ProTxReg but to get CKeyID since then which we can do by simply decoding an address instead. This simplifies masternode registration process by letting an Operator to issue `protx register_prepare` on its own instead of asking an Owner. An Owner still have to sign the message provided by an Operator with his collateral address to prove collateral ownership (and authorize masternode registration). Note: `protx register_*` rpc will no longer accept privkeys for ownerAddress. Some 3-rd party software like DMT might need to be patched to work correctly with nodes running with this fix.

Implement CDBTransaction and CScopedDBTransaction

4531f6b

Allows easier commit/rollback handling, especially useful when AcceptBlock fails and things need to be reverted.

UdjinM6 added this to the 12.4 milestone Aug 30, 2018

UdjinM6 requested changes Aug 30, 2018

View reviewed changes

codablock force-pushed the pr_dip3 branch 3 times, most recently from 995268b to 72336d2 Compare August 30, 2018 14:54

UdjinM6 requested changes Aug 30, 2018

View reviewed changes

schinzelh reviewed Aug 30, 2018

View reviewed changes

Comment thread src/evo/evodb.h Outdated

schinzelh Aug 30, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you should update your template to 2018 :)

UdjinM6 reacted with laugh emoji

codablock added 10 commits August 30, 2018 19:47

Introduce CEvoDB for all evo related things, e.g. DIP3

c9a72e8

Also add transaction handling to ConnectTip and DisconnectTip and a few other places where blocks are processed.

Implementation of ProRegTx with basic validation (no processing)

958b84a

Implement "protx register" RPC

c772423

Conflict handling for ProRegTx in mempool

cdd723e

Automatically lock ProTx collaterals when TX is added/loaded to wallet

76fd308

Avoid accidential spending of ProTx collaterals.

Implementation of deterministic MNs list

9e8a867

Add spork to control deterministic MN lists activation

5461e92

Don't allow non-ProTx masternode collaterals after DIP3 activation

2c17287

Implement "protx list" RPC

5e3abec

Update copyright in specialtx.h/cpp

c566ce7

codablock force-pushed the pr_dip3 branch from 72336d2 to c566ce7 Compare August 30, 2018 18:02

UdjinM6 approved these changes Aug 30, 2018

View reviewed changes

Comment thread src/evo/deterministicmns.h Outdated

UdjinM6 Aug 30, 2018

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, the argument about organising cache this way makes sense, I agree.

UdjinM6 merged commit 74629e4 into dashpay:develop Aug 30, 2018

UdjinM6 mentioned this pull request Oct 13, 2020

rpc: Do not require a privkey corresponding to ownerAddress in protx register_* to be known by the wallet #3773

Merged

Uh oh!

Conversation

codablock commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codablock Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codablock commented Aug 30, 2018

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codablock commented Aug 30, 2018

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codablock commented Aug 30, 2018 •

edited

Loading

codablock Aug 30, 2018 •

edited

Loading