Tpos signature hf #154
Tpos signature hf #154
Conversation
…. Changed how block signature checks are performed
…or 0 value commissions. Added foundation for new tpos contract and new signature
… contract. Refactoring
| @@ -45,15 +46,18 @@ bool CBlockSigner::SignBlock() | |||
|
|
|||
| if(refBlock.IsTPoSBlock()) | |||
| { | |||
| CKeyID merchantKeyID; | |||
| if(!refContract.merchantAddress.GetKeyID(merchantKeyID)) | |||
| if(!refContract.scriptMerchantAddress.IsPayToPublicKeyHash()) { | |||
There was a problem hiding this comment.
Would you mind elaborating why is this required?
There was a problem hiding this comment.
Currently we accept merchant addresses only in P2PKH format, so we avoid signing blocks that would get rejected
src/blocksigner.cpp
Outdated
| return CHashSigner::SignHash(refBlock.IsTPoSBlock() ? refBlock.GetTPoSHash() : refBlock.GetHash(), keySecret, scriptType, refBlock.vchBlockSig); | ||
|
|
||
| const auto &hash = refBlock.IsTPoSBlock() ? refBlock.GetTPoSHash() : refBlock.GetHash(); | ||
| if (nChainHeight >= Params().GetConsensus().nTPoSSignatureUpgradeHFHeight) { |
There was a problem hiding this comment.
I'd suggest to create a common function that has tests to check this, instead of keep writing the same check everywhere.
Or, even better, just write a helper thing that handles signing contracts, and verifying signatures.
src/blocksigner.cpp
Outdated
| return true; | ||
| } | ||
|
|
||
| std::string strError; | ||
| return CHashSigner::VerifyHash(hashMessage, destination, refBlock.vchBlockSig, strError); | ||
| if (nChainHeight >= Params().GetConsensus().nTPoSSignatureUpgradeHFHeight) { |
There was a problem hiding this comment.
Same duplication stated above.
src/chainparams.cpp
Outdated
| @@ -99,6 +99,7 @@ class CMainParams : public CChainParams { | |||
| consensus.nPosTargetSpacing = 1 * 60; // XSN: 1 minutes | |||
| consensus.nPosTargetTimespan = 60 * 40; | |||
| consensus.nPoSUpdgradeHFHeight = 898488; // 4 December 2019 | |||
| consensus.nTPoSSignatureUpgradeHFHeight = 1264447; | |||
There was a problem hiding this comment.
It's working commenting what's this for, and why.
There was a problem hiding this comment.
we will set the date later
| base58Prefixes[SECRET_KEY] = std::vector<unsigned char>(1,239); | ||
| base58Prefixes[EXT_PUBLIC_KEY] = {0x04, 0x35, 0x87, 0xCF}; | ||
| base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94}; | ||
| base58Prefixes[PUBKEY_ADDRESS] = std::vector<unsigned char>(1,76); |
There was a problem hiding this comment.
Are these changes actually needed? It was useful for testing but is it ok to keep the change? Also, I'd add the details to the README for using the regtest.
There was a problem hiding this comment.
Yeah, should be ok, doesn't look too bad
| } else if (tokens.size() > 3 && tokens.size() < 6) { | ||
|
|
||
| CBitcoinAddress tposAddress(ParseAddressFromMetadata(tokens[1])); | ||
| CBitcoinAddress merchantAddress(ParseAddressFromMetadata(tokens[2])); |
There was a problem hiding this comment.
merchant address is not P2PKH
I see you are validating this in another file, shouldn't you do the same to prevent creating such invalid contracts?
src/tpos/tposutils.cpp
Outdated
| commission, vchSignature); | ||
| contract.nVersion = 1; // legacy version | ||
|
|
||
| if(contract.IsValid() && commission > 0 && commission < 100) { |
There was a problem hiding this comment.
These checks could go in a common function to avoid getting confused on whether you accept 0 or not.
src/validation.cpp
Outdated
| TPoSContract contract; | ||
| std::string strError; | ||
| if (!TPoSUtils::CheckContract(ptx, contract, chainActive.Height(), true, false, strError)) { | ||
| return state.Invalid(false, REJECT_INVALID, "tpos-invaid-contract"); |
| static const int PROTOCOL_VERSION = 70209; | ||
| static const int PROTOCOL_VERSION = 70210; | ||
|
|
||
| static const int PRETPOS_SIGNTURE_FIX_PROTO_VERSION = 70209; |
There was a problem hiding this comment.
It's worth adding a comment.
| percentage = tposContract.stakePercentage; // first vout is owner | ||
|
|
||
| if(tposContract.IsValid()) { | ||
| percentage = (100 - tposContract.nOperatorReward); // first vout is owner |
There was a problem hiding this comment.
A reusable function would help.
…s-signature-hf
The goal is to allow Trezor 1 to create TPoS contracts for Stakenet. Last year, Stakenet introduced a hard-fork [1] to change the way TPoS contracts are created, instead of a custom signature method, now it works with the output from the signMessage method, while this works for Trezor T, it doesn't work for Trezor 1 due to the 80 bytes limit on the OP_RETURN output while Stakenet allows up to 150 bytes [2], in a gitter discussion [3] we concluded that the change should be fine. The hard-fork was introduced because we couldn't got our TPoS contracts PR accepted [4], the OP_RETURN still contains the same data, its just stored in a different way: - The TPoS address, where the coins to stake are stored, and where rewards are received. - The merchant address, where the merchant receives its commission. - The contract commission. - The TPoS collateral signature (this is what uses the signMessage now). At last, there is an example transaction creating a TPoS contract [5]. [1]: X9Developers/XSN#154 [2]: https://github.com/X9Developers/XSN/blob/master/src/script/standard.h#L34 [3]: https://gitter.im/trezor/community?at=6064c41e940f1d555e2ea670 [4]: #140 [5]: https://xsnexplorer.io/transactions/858feb31097501cf68d698cde104cf778ec51ff3668e943404b549a5dd2f5792
No description provided.