Manual masternode vaults #45
Conversation
domob1812
force-pushed
the
mnvault-test
branch
3 times, most recently
from
6a13d9b to
6696f0c
Compare
domob1812
force-pushed
the
mnvault-test
branch
3 times, most recently
from
a177570 to
808d253
Compare
domob1812
force-pushed
the
mnvault-test
branch
2 times, most recently
from
c36e28e to
2c48d60
Compare
|
This is mostly ready, except we should make use of custom reward addresses (#44) to make the new regtest more extensive. Thus that one should probably be merged first. |
domob1812
force-pushed
the
mnvault-test
branch
from
2c48d60 to
97ad1cc
Compare
domob1812
force-pushed
the
mnvault-test
branch
2 times, most recently
from
9e7b687 to
48779e4
Compare
domob1812
force-pushed
the
mnvault-test
branch
2 times, most recently
from
a0aa7f3 to
a3ed211
Compare
|
Let's extend the test with custom reward addresses in a follow-up. The other changes in here are independent of #44, so this is ready to review and merge (should be mostly straight forward). |
domob1812
force-pushed
the
mnvault-test
branch
from
a3ed211 to
f973f44
Compare
domob1812
force-pushed
the
mnvault-test
branch
from
f973f44 to
c4f4f0d
Compare
domob1812
force-pushed
the
mnvault-test
branch
from
c4f4f0d to
baaf9b8
Compare
galpHub
left a comment
galpHub
left a comment
There was a problem hiding this comment.
Yep. This one is basically ready. Just need to fix the time offset to avoid any p2p issues with other broadcast use-cases.
domob1812
force-pushed
the
mnvault-test
branch
from
a23f3f3 to
0d2eb3d
Compare
|
Thanks for the review, I've updated the time bump now and also added a fix for a potential deadlock, where we reduce the time held onto |
divi/src/BlockFactory.cpp
Outdated
|
|
||
| pblocktemplate->previousBlockIndex = chain_.Tip(); | ||
| { | ||
| LOCK(mainCS_); |
There was a problem hiding this comment.
We may be able to drop the lock altogether. I was doing some digging into it, and since the chain tip is already captured and passed as a parameter downstream, the lock isnt really required. Moreover, since we're checking the chainTip for being non-null, it would function correctly. Do you have any objections to removing this lock altogether?
There was a problem hiding this comment.
I think strictly speaking the lock is required, since CChain::Tip() performs some operations on std::vector (like operator[]). And those are likely not thread-safe, e.g. the vector could just be in the process of being reallocated or something. My guess is that in practice it will most likely work almost always, but in theory it could lead to weird bugs.
It might be possible to make CChain::Tip() thread safe by caching the tip pointer and using e.g. std::atomic inside CChain, which is something we could do at some point if you are worried about the lock. But in the current state, I don't think it would be (completely) safe to remove the lock.
In the end, this is your decision, though. As I said, I don't think it would cause a lot of issues in practice; so if you think we should remove the lock, there are no objections from my side.
domob1812
force-pushed
the
mnvault-test
branch
from
0d2eb3d to
0a44f8e
Compare
domob1812
force-pushed
the
mnvault-test
branch
3 times, most recently
from
e39638b to
1d5d841
Compare
domob1812
force-pushed
the
mnvault-test
branch
2 times, most recently
from
9dc2504 to
30ff7e8
Compare
When creating a delayed masternode broadcast, check the sigtime against the collateral confirmation block and make sure that it meets the network criterion (signature must not be earlier than the timestamp of the block that confirmed the collateral 15 times). If the condition is violated, just use "now + one hour" as sigtime.
broadcaststartmasternode has a feature to refresh the ping's timestamp if an explicit signature is passed. This extends that feature to allow passing "update_ping" in place of the signature, in which case the signature contained in the mnb will be kept but the ping will still be refreshed. Not passing a second argument at all is still a way to just broadcast the message as-is.
This adds a new regtest mnvaults.py, which manually implements the proposal for masternode vaults with a temporary key and a pre-signed unvault transaction. It sets up a masternode with a collateral at a temporary address, for which we need not retain the private key. Instead we just keep a pre-signed transaction to withdraw the funds to a specified address and the signed masternode broadcast in storage, and use them to first run the masternode and then unvault the funds.
|
Reopened as #81 with fixed branches. |
2 participants
This implements a new regtest
mnvaults.py, which runs through a "masternode vault" workflow with a presigned unvaulting transaction and a presigned masternode broadcast (so that the collateral private key need not be kept around). For now, all is done "manually" in the test, without specific support for vaulting/unvaulting fromdivid.This also contains a bunch of tweaks, simplifications and fixes to the general masternode logic and RPC methods that are necessary to make it work.