Disable signature rule in IBD

[quantumagi]

See https://app.clickup.com/t/40ra67.

[fassadlr]

@quantumagi please check as the solution doesnt build :)

[fassadlr]

Please fix build

[fassadlr]

@noescape00 I believe you mentioned that this will suffice for us to increase the the sync speed a bit? Can we have your comments?

[noescape00]

I'm against removing signature rule since this rule is the only thing that secures poa network.
However we can skip this rule prior to last checkpoint.

[fassadlr]

@noescape00 what about skipping until the node is out of IBD?

[noescape00]

@noescape00 what about skipping until the node is out of IBD?

Yes. IBD is syncing prior to last checkpoint.

upd
it's incorrect, in our case ibd is also after last checkpoint

[noescape00]

Instead of checking IsIBD check just if below last checkpoint.

if (this.chainState.ConsensusTip.Header.BlockTime < (this.dateTimeProvider.GetUtcNow().AddSeconds(-this.consensusSettings.MaxTipAge)))
            {
                if (!this.network.IsRegTest())
                    return true;

                // RegTest networks may experience long periods of no mining.
                // If this happens we don't want to be in IBD because we can't
                // mine new blocks in IBD and our nodes will be frozen at the
                // current height. Also note that in RegTest its typical for one
                // machine to control all (local) nodes and hence we are in control
                // of any side-effects that may arise from returning false here.
            }

Because of this code in IsInitialBlockDownload method we would be skipping signature rule after last checkpoint

Doing so will result in vulnerability when attacker provides chain that is correct prior to last checkpoint and then has more blocks than the correct chain but those are invalid.

[quantumagi]

@noescape00 , i've made some changes based on your comment.

[fassadlr]

I don't think we need to check IBD anymore if we are only checking the last checkpoint?

[fassadlr]

IMO this if can be removed.

[quantumagi]

It's the better of the two options when one considers that a Guard.Assert(this.initialBlockDownloadState.IsInitialBlockDownload()) would be appropriate. We have to be sure that this is all happening in IBD as expected.

[fassadlr]

Perhaps this should be if the current block's height being validated matches the last checkpoint height, THEN check the hash?

@noescape00 can you confirm?

[quantumagi]

This is already implicitly being checked by the preceding ifs - i.e. <= except <.

[noescape00]

ChainedHeaderToValidate here might be any block below last checkpoint. So most of the time it wont be equal to last checkpoint hash.

Also no need to throw invalid header signature since it might be valid, just the block is not checkpointed.

So probably just ignore the sig validation rule below last checkpoint, everything else is not needed.

[quantumagi]

ChainedHeaderToValidate here might be any block below last checkpoint. So most of the time it wont be equal to last checkpoint hash.

The code takes that into account by only checking the last checkpoint block. Look carefully: First I look at <= and then I return on the < case. Thereafter the code is only executing for the = case.

Note that blocks following the last checkpoint block can only be deemed valid if we fully validate the checkpoint block when we encounter it. Not only its height must match but also its hash. This way we know 100% we are not validating signatures on some fake chain perhaps built by a small subset of miners. I.e. it definitely won't hurt doing this check.

[noescape00]

Not sure why we need to do it here since there is a rule already that checks checkpoints

[quantumagi]

Can you point me at which rule that is? I was trying to find a file named XXXRule.cs that contains "Checkpoint" and only found PosCoinviewRule - but that's not it, right?

[fassadlr]

@quantumagi this is the code @noescape00 is referring to in CHT:

        private ConnectNewHeadersResult HandleCheckpointsHeader(ChainedHeader chainedHeader, ChainedHeader latestNewHeader, CheckpointInfo checkpoint, int peerId)
        {
            if (checkpoint.Hash != chainedHeader.HashBlock)
            {
                // Make sure that chain with invalid checkpoint in it is removed from the tree.
                // Otherwise a new peer may connect and present headers on top of invalid chain and we wouldn't recognize it.
                this.RemovePeerClaim(peerId, latestNewHeader);

                this.logger.LogDebug("Chained header '{0}' does not match checkpoint '{1}'.", chainedHeader, checkpoint.Hash);
                this.logger.LogTrace("(-)[INVALID_HEADER_NOT_MATCHING_CHECKPOINT]");
                throw new CheckpointMismatchException();
            }

[fassadlr]

2 small changes and then we good to go once build passes