PH integrity validation improvements + gateway fix #3726
Conversation
| // In case we are in PoW era there might be no coinstake tx. | ||
| // We have no way of telling if the block was supposed to be PoW or PoS so attacker | ||
| // can trick us into thinking that all of them are PoW so no PH is required. | ||
| if (header == null || !header.Coinstake.IsCoinStake) |
There was a problem hiding this comment.
I actually don't understand why the need to fail in case we are bellow the checkpoint.
If this is a regular header this rule does not need to do anything just return.
The purpose of this rule is only to ensure the PH coinstake and bloc coinstake are equal, the other checks don't belong to this rule (and some may even be duplicates)
There was a problem hiding this comment.
I'm wondering if we should remove this rule when the node is setup as gateway
There was a problem hiding this comment.
It will be actually more complicated to do that.
|
This PR is totally bloated for no reason, injecting the connection for the settings is really not needed. |
noescape00
force-pushed
the
integrityRulesFix
branch
from
15c9d10 to
f82d3ef
Compare
|
Agreed with @dangershony . |
zeptin
dismissed
their stale review
I think that after the conflicts are resolved this can be re-looked at for merging
| namespace Stratis.Bitcoin.Features.Consensus.Rules.CommonRules | ||
| { | ||
| /// <summary>Checks that coinstake tx from the proven header is the 2nd transaction in a block and that the block has at least 2 transactions.</summary> | ||
| public class PosTransactionsOrderRule : IntegrityValidationConsensusRule |
There was a problem hiding this comment.
Is this the right name?
Looks like the main check of this Rule is that the coinstake on the block and on the proven header match.
There re is no regard to the order of transactions
| /// <exception cref="ConsensusErrors.InvalidTxCount">Thrown in case block has less than 2 transaction.</exception> | ||
| /// <exception cref="ConsensusErrors.PHCoinstakeMissmatch">Thrown in case coinstake transaction from the proven header missmatches 2nd block transaction.</exception> | ||
| /// <exception cref="ConsensusErrors.ProofOfWorkTooHigh">Thrown in case block is a PoW block but created after the last pow height.</exception> | ||
| public override void Run(RuleContext context) |
There was a problem hiding this comment.
@noescape00 I would split this rule into three separately rules. The logic looks good but it might be better to have the logic split then we can name each rule properly according to its purpose.
There was a problem hiding this comment.
I believe there already is a rue to check for ProofOfWorkTooHigh
Dont merge before the release
see #3720