Include protocol version into MNAUTH #3631
Conversation
| // This is ok as we only use MNAUTH as a DoS protection and not for sensitive stuff | ||
| signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound)); | ||
| int nOurNodeVersion{PROTOCOL_VERSION}; | ||
| if (Params().NetworkIDString() != CBaseChainParams::MAIN && gArgs.IsArgSet("-pushversion")) { |
There was a problem hiding this comment.
I presume we don't intend to keep this, so maybe add a TODO to remove in either rc4 or final 0.16
There was a problem hiding this comment.
Nope, this is needed because we imitate old nodes in feature_llmq_simplepose.py and they should send mnauth in correct format cause otherwise other nodes won't let them connect. Same for ProcessMessage - if we pretend to be an older node we should be expecting other nodes to send us mnauth in old format.
src/evo/mnauth.cpp
Outdated
| if (pnode->nVersion < MIN_MASTERNODE_PROTO_VERSION || nOurNodeVersion < MIN_MASTERNODE_PROTO_VERSION) { | ||
| signHash = ::SerializeHash(std::make_tuple(*activeMasternodeInfo.blsPubKeyOperator, pnode->receivedMNAuthChallenge, pnode->fInbound)); | ||
| } else { |
There was a problem hiding this comment.
I feel like we shouldn't be using MIN_MASTERNODE_PROTO_VERSION here, because assume in v17 we bump min masternode proto, then a v17 node communicating with a v16 node will think that the v16 node doesn't understand version and would exclude it. We should add a new variable representing the protocol that MnAuth was changed, and then this can be removed once min proto version < mnauthaddedprotocol
|
Thats a good one, utACK |
* Include protocol version into MNAUTH * Introduce MNAUTH_NODE_VER_VERSION = 70218
|
backported in #3670 |
* Include protocol version into MNAUTH * Introduce MNAUTH_NODE_VER_VERSION = 70218
3 participants
(As discussed in Slack a couple days ago) now that PoSe relies on a protocol version send by other MNs, it makes sense to include it into
MNAUTHmessage to make sure it wasn't forged by a mitm attacker somehow.Note: bumping
MIN_MASTERNODE_PROTO_VERSIONwill trigger another PoSe-score increase/ban wave for currently deployed testnet nodes running on 70217 once enough nodes are upgraded to this commit which is fine and might even be a desirable thing cause it allows us to test PoSe during protocol bumps once again.