-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
M-of-N-like sporks #2288
M-of-N-like sporks #2288
Conversation
Thanks for this implementation @gladcow! I'm not sure it makes sense to try and coordinate on
I don't understand this. What do you mean by "because votes of first 3 signers don't allow any node to calculate current spork value" ? Can you explain this in another way? I don't think the intention is to require signing and broadcasting at a specific time or timestamp, but rather, sporks are updated per-signer and when the threshold number is reached where ID / Value match, then the active spork switches to that value. |
@nmarley ,
Let's consider what a some usual node has in this situation. It has spork messages from signers 1-3 with spork value |
I think I see what's going on here with switching from time to sequence id - it's basically an attempt to make sure everyone is signing the same message I guess. But I don't see why it would matter if signatures are not combined (in a crypto way like e.g. in BLS). To distinguish messages signed by different spork key holders I'd rather switch from |
I initially was trying to do something like this, but I don't think it's necessary (is an arbitrary unique ID field that doesn't have to be honored by all nodes and is superfluous). We can just use the sporkAddr as the unique ID instead. The spork signer is identified by reading the signature and recovering the pubkey / hash160, something like: |
@nmarley Ah, good point re pubkey recovery! |
Looks like I can't explain it clearly.) This 3 signers and other 2 signers participate in different votings really, for example first 3 signers vote in 2017 year and last 2 signers vote in 2018. And spork should be It is not attempt to simply defend the written code, it is a real problem I've encountered during testing.) |
Ah, ok, thanks for the explanation. I think I understand now. In this case, just make the threshold value require a majority, e.g. if 5 total spork keys exist, require at least 3 of them. That should solve the problem, right? |
@nmarley , right, it solves the problem, but I've thought that |
I'd say let's start with implementing the simplest/smallest possible one aka 2of3 first and see how it goes. |
So, to make it clear, what should be changed:
Have I missed something? |
@gladcow sounds good (assuming (4) means ", support providing multiple |
Rebased and fixed, re-review, please. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about all the nits! See inline
src/init.cpp
Outdated
if (!sporkManager.SetSporkAddress(GetArg("-sporkaddr", Params().SporkAddress()))) | ||
return InitError(_("Invalid spork address specified with -sporkaddr")); | ||
std::vector<std::string> vSporkAddresses; | ||
if(mapMultiArgs.count("-sporkaddr")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(
-> if (
src/init.cpp
Outdated
} else { | ||
vSporkAddresses = Params().SporkAddresses(); | ||
} | ||
for(const auto& address: vSporkAddresses) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for(
-> for (
} | ||
for(const auto& address: vSporkAddresses) { | ||
if (!sporkManager.SetSporkAddress(address)) | ||
return InitError(_("Invalid spork address specified with -sporkaddr")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bodies of an if should either be on the same line or in brackets
If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces. In every other case, braces are required, and the then and else clauses must appear correctly indented on a new line.
|
||
int minsporkkeys = GetArg("-minsporkkeys", Params().MinSporkKeys()); | ||
if(!sporkManager.SetMinSporkKeys(minsporkkeys)) | ||
return InitError(_("Invalid minimum number of spork signers specified with -minsporkkeys")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above, same line or braces. Also if(
-> if (
src/spork.cpp
Outdated
{ | ||
LOCK(cs); | ||
if (!mapSporksActive.count(sporkID)) | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
above, same line or braces
src/spork.cpp
Outdated
// check if any value has enough signer votes | ||
int max_count = 0; | ||
for (const auto& value_data: value_counts) { | ||
if(value_data.second >= nMinSporkKeys) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(
-> if (
src/spork.cpp
Outdated
@@ -55,41 +81,54 @@ void CSporkManager::ProcessSpork(CNode* pfrom, const std::string& strCommand, CD | |||
if(!chainActive.Tip()) return; | |||
strLogMsg = strprintf("SPORK -- hash: %s id: %d value: %10d bestHeight: %d peer=%d", hash.ToString(), spork.nSporkID, spork.nValue, chainActive.Height(), pfrom->id); | |||
} | |||
|
|||
CKeyID signer; | |||
if(!(spork.GetSignerKeyID(signer, IsSporkActive(SPORK_6_NEW_SIGS)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(
-> if (
src/spork.cpp
Outdated
} else { | ||
LogPrint("spork", "CSporkManager::IsSporkActive -- Unknown Spork ID %d\n", nSporkID); | ||
r = 4070908800ULL; // 2099-1-1 i.e. off by default | ||
if(SporkValueIsActive(nSporkID, r)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(
-> if (
)){
-> )) {
src/spork.cpp
Outdated
if (mapSporksActive.count(nSporkID)) | ||
return mapSporksActive[nSporkID].nValue; | ||
int64_t r = -1; | ||
if(SporkValueIsActive(nSporkID, r)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(
-> if (
)){
-> )) {
src/spork.cpp
Outdated
return r < GetAdjustedTime(); | ||
} | ||
|
||
// grab the value of the spork on the network, or the default | ||
int64_t CSporkManager::GetSporkValue(int nSporkID) | ||
{ | ||
LOCK(cs); | ||
if (mapSporksActive.count(nSporkID)) | ||
return mapSporksActive[nSporkID].nValue; | ||
int64_t r = -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer a better name ¯\(ツ)/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PastaPastaPasta , I'm writing wrong if
s automatically, I think it is bad practice to break codestyle guide, so it is good you are pointing this errors to me, I'm sorry that I'm making them so often.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see inline comments
qa/rpc-tests/multikeysporks.py
Outdated
Test logic for several signer keys usage for spork broadcast. | ||
|
||
We set 5 possible keys for sporks signing and set minimum | ||
required signers to 2. We check 1 siger can't set the spork |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be updated to match actual test logic.
qa/rpc-tests/multikeysporks.py
Outdated
# first and second signers set spork value | ||
self.set_test_spork_state(self.nodes[0], 1) | ||
self.set_test_spork_state(self.nodes[1], 1) | ||
# spork change requires at least 2 signers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
src/spork.cpp
Outdated
} | ||
|
||
LogPrint("spork", "CSporkManager::IsSporkActive -- Unknown Spork ID %d\n", nSporkID); | ||
r = 4070908800ULL; // 2099-1-1 i.e. off by default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to set r
here, could just return false instead.
src/spork.h
Outdated
CKey sporkPrivKey; | ||
|
||
bool SporkValueIsActive(int sporkID, int64_t& activeValue) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming: nSporkID
and nActiveValueRet
(same in cpp)
src/spork.cpp
Outdated
int max_count = 0; | ||
for (const auto& value_data: value_counts) { | ||
if (value_data.second >= nMinSporkKeys) { | ||
if (value_data.second > max_count) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the conditions in SetMinSporkKeys
there can be only one item with value_data.second >= nMinSporkKeys
i.e. there is no need for max_count
and if
, you can simply set activeValue
and return true;
here. Or even better, you could check this in previous loop (line 40) and drop this one completely.
src/spork.cpp
Outdated
if (!mapSporksActive.count(sporkID)) return false; | ||
|
||
// calc how many values we have and how many signers vote for every value | ||
std::map<int64_t, int> value_counts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming: mapValueCounts
src/spork.cpp
Outdated
@@ -55,41 +81,54 @@ void CSporkManager::ProcessSpork(CNode* pfrom, const std::string& strCommand, CD | |||
if(!chainActive.Tip()) return; | |||
strLogMsg = strprintf("SPORK -- hash: %s id: %d value: %10d bestHeight: %d peer=%d", hash.ToString(), spork.nSporkID, spork.nValue, chainActive.Height(), pfrom->id); | |||
} | |||
|
|||
CKeyID signer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming: keyIDSigner
src/spork.cpp
Outdated
} | ||
spork.Relay(connman); | ||
|
||
//does a task if needed | ||
ExecuteSpork(spork.nSporkID, spork.nValue); | ||
int64_t activeValue = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nActiveValue
src/spork.cpp
Outdated
@@ -129,10 +168,15 @@ bool CSporkManager::UpdateSpork(int nSporkID, int64_t nValue, CConnman& connman) | |||
CSporkMessage spork = CSporkMessage(nSporkID, nValue, GetAdjustedTime()); | |||
|
|||
if(spork.Sign(sporkPrivKey, IsSporkActive(SPORK_6_NEW_SIGS))) { | |||
CKeyID signer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keyIDSigner
if (!(s.GetType() & SER_GETHASH)) { | ||
READWRITE(vchSig); | ||
} | ||
READWRITE(vchSig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the hash. Not sure if this is going to work (without being banned), will test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'm being banned on testnet. Should probably change GetSignatureHash()
to hash everything but vchSig
and use it instead of GetHash()
in GetSignerKeyID()
(for activated spork6 only).
if (!(s.GetType() & SER_GETHASH)) { | ||
READWRITE(vchSig); | ||
} | ||
READWRITE(vchSig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I'm being banned on testnet. Should probably change GetSignatureHash()
to hash everything but vchSig
and use it instead of GetHash()
in GetSignerKeyID()
(for activated spork6 only).
src/spork.cpp
Outdated
{ | ||
CPubKey pubkeyFromSig; | ||
if (fSporkSixActive) { | ||
if (!pubkeyFromSig.RecoverCompact(GetHash(), vchSig)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use a (modified) GetSignatureHash()
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hash should include signature for this functionality because of the fact that signature is the only thing that is different in messages from different signers. If we don't include the signature the messages from other signers are not broadcasted after the message from the first signer. So it looks like I should add some update code compatible with the old version.(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know :) But this is only true for p2p part, not for the sign/verify part which can use whatever hash it needs, that's basically the purpose of GetSignatureHash()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see now, will try to fix it this way.)
Rebased and fixed, re-review, please |
Contains changes from #2313 (I missed them), but also stops to serialize pubkey ids, pubkeys should be hardcoded or read from options, not from spork cache ( = from previous dashd run) |
@nmarley A little late and probably not relevant at this point in time, but my understanding is that this type of pubkey recovery cannot be done with BLS. Perhaps something to think about in the future. |
Pls rebase again and bump |
…e synced at moment
Rebased, serialization version is bumped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a couple of (small) change requests, mostly about readability, see inline commits. Otherwise looks good and seems to be working 👍 Will test a bit more.
src/spork.cpp
Outdated
if (!itActive->second.CheckSignature(sporkPubKeyID, true)) { | ||
mapSporksByHash.erase(itActive->second.GetHash()); | ||
mapSporksActive.erase(itActive++); | ||
auto signer_msg_pair = itActive->second.begin(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name is confusing here imo, would prefer smth like itSignerPair
or smth like that.
src/spork.cpp
Outdated
continue; | ||
} | ||
} | ||
signer_msg_pair++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use pre-increment here i.e. ++signer_msg_pair;
(or rather ++itSignerPair;
considering the previous comment)
src/spork.cpp
Outdated
|
||
} else if (strCommand == NetMsgType::GETSPORKS) { | ||
LOCK(cs); // make sure to not lock this together with cs_main | ||
for (const auto& pair : mapSporksActive) { | ||
connman.PushMessage(pfrom, CNetMsgMaker(pfrom->GetSendVersion()).Make(NetMsgType::SPORK, pair.second)); | ||
for (const auto& signer_spork: pair.second) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer a bit more self-describing name e.g. signerSporkPair
src/spork.cpp
Outdated
{ | ||
int maxKeysNumber = setSporkPubKeyIDs.size(); | ||
if ((minSporkKeys <= maxKeysNumber / 2) || (minSporkKeys > maxKeysNumber)) { | ||
LogPrintf("CSporkManager::SetSporkAddress -- Invalid min spork signers number: %d\n", minSporkKeys); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/SetSporkAddress /SetMinSporkKeys/
src/spork.cpp
Outdated
@@ -88,41 +127,60 @@ void CSporkManager::ProcessSpork(CNode* pfrom, const std::string& strCommand, CD | |||
if(!chainActive.Tip()) return; | |||
strLogMsg = strprintf("SPORK -- hash: %s id: %d value: %10d bestHeight: %d peer=%d", hash.ToString(), spork.nSporkID, spork.nValue, chainActive.Height(), pfrom->id); | |||
} | |||
|
|||
CKeyID keyIDSigner; | |||
if (!(spork.GetSignerKeyID(keyIDSigner, IsSporkActive(SPORK_6_NEW_SIGS)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer this rewritten via ||
src/spork.cpp
Outdated
@@ -162,10 +220,15 @@ bool CSporkManager::UpdateSpork(int nSporkID, int64_t nValue, CConnman& connman) | |||
CSporkMessage spork = CSporkMessage(nSporkID, nValue, GetAdjustedTime()); | |||
|
|||
if(spork.Sign(sporkPrivKey, IsSporkActive(SPORK_6_NEW_SIGS))) { | |||
CKeyID keyIDSigner; | |||
if (!(spork.GetSignerKeyID(keyIDSigner, IsSporkActive(SPORK_6_NEW_SIGS) && setSporkPubKeyIDs.count(keyIDSigner)))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer this rewritten via ||
Fixed the issues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly tested ACK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK, 👍 Good work @gladcow !
Seems like an easy transition from legacy to multiple keys, by:
- Adding multiple keys by default in 12.4 release (with legacy key being 1 of them) and only requiring 1-of-1
- Coordinating and signing sporks with other (non-legacy) keys during that period
- The next release after that can remove or replace the default spork key (since sporks now signed by multiple keys), and require majority.
Is this what you had in mind for rollout @UdjinM6 ?
Not sure I understand it right @nmarley. I was thinking of using another spork to activate new keys smth like
So first activate SPORK_X_MULTIKEYSPORK with the legacy key and then re-sign needed sporks with multiple keys. |
Wouldn't that be more complicated? Seems to me like the method I outlined would be easier, and it doesn't require another spork (which would only be used for this purpose). Are there any downsides to that method? |
@nmarley I don't think the logic for spork activation here is compatible with |
@UdjinM6
Something like: nmarley/dash@pr2288-rebased...spork-legacy-logic Should be separate PR if we go that route. I think this particular PR should be good though as-is, since it introduces the main multi signers logic and there could be alternative ways to activate, as we've discussed a couple already. |
edit: Actually, not exactly this:
More like, prefer multi signer logic first, and then fall back to legacy spork key. |
Need to think more about it but I agree that new keys and migration logic is smth for another PR. Let's merge this as is first :) |
See issue #2221
allow multiple hardcoded spork addresses (N) to be specified in chainparams/cmd-line/config (same signers for all sporks);
allow multiple messages with the same id/value signed by different spork addresses;
(de)activate any spork only when at least M similar message with the same id/value signed by different signers were received (the same M for all sporks);
"M-of-N" spork implementation requires spork signers consensus achieved with some external communication and doesn't offer any facilities for such consensus achievement. If this consensus is not achived, the spork is treated as it has default value. For example, if sporks are configured as "at least 2 signers of 5 possible" and some spork has default value
0
, and 2 signers broadcast this spork value1
, while 2 other signers broadcast value2
(andnTimeSigned
field has the same value, see details later), this spork is treated as it has value0
.Also, several possible signers require new logic for
nTimeSigned
field processing. In old implementation this field is used to separate old non-actual spork messages from the new spork messages (spork message with biggernTimeSigned
overwrites message with smaller one). We need similar logic in new implementation. For example, we have the same configuration like in previous example, 2-of-5-signers. At momentt1
signers 1, 2, 3 broadcast spork with value1
and spork switches to this value. After that at momentt2
signers 4 and 5 tries to switch this spork to value2
, and fail to do this, because votes of first 3 signers don't allow any node to calculate current spork value, and spork goes to default value. We can't also simply overwrite all "old" spork messages of other signers because of the fact that in this case just one signer can "rallback" several other signer's messages.To resolve this issue we can require all signers to use the same value of the
nTimeSigned
field to treat their messages as a part of the same "spork broadcast" and allow such signer's groups to overwrite messages of other signer's groups with lessnTimeSigned
field value if this group size is bigger or equal to the minimum configured value (M in M-of-N). It is difficult to use current time for this goal as it was before, because signer's clocks are not synchronized and signers can broadcast their messages at slightly different moments of time. I think it is better to use increasing counter instead and allow signers to set its value directly. Also it is better to allow spork signers to use some default way to calculate this counter value for their convenience and I have added such calculation, but this calculation is not a facility for achieving of signers consensus, so it doesn't resolve all possible situations and sometimes signers should set counter value explicitly. Also, I've renamednTimeSigned
field tonBroadcastID
to reflect its new meaning.