bosagora / agora

POC Node implementation for CoinNet
https://bosagora.io
MIT License
36 stars 22 forks source link

Prevent headers which are signed by slashed validators #3297

Closed hewison-chris closed 2 years ago

hewison-chris commented 2 years ago

Fixes #3296. First commit ensures that we would not accept a header in this situation (as we receive it from other nodes). The second commit prevents the situation where these headers are generated.

codecov[bot] commented 2 years ago

Codecov Report

Merging #3297 (80fb953) into v0.x.x (ef399df) will decrease coverage by 0.03%. The diff coverage is 42.85%.

@@            Coverage Diff             @@
##           v0.x.x    #3297      +/-   ##
==========================================
- Coverage   88.79%   88.75%   -0.04%     
==========================================
  Files         165      165              
  Lines       17063    17067       +4     
==========================================
- Hits        15151    15148       -3     
- Misses       1912     1919       +7     
Flag Coverage Δ
integration 40.02% <ø> (+0.13%) :arrow_up:
unittests 87.83% <42.85%> (-0.03%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/agora/consensus/state/Ledger.d 89.18% <20.00%> (-1.55%) :arrow_down:
source/agora/consensus/protocol/Nominator.d 91.47% <100.00%> (+0.54%) :arrow_up:
source/agora/network/RPC.d 52.52% <0.00%> (-2.03%) :arrow_down:
source/agora/node/Validator.d 91.95% <0.00%> (-1.01%) :arrow_down:
source/agora/network/Manager.d 76.89% <0.00%> (-0.32%) :arrow_down:
source/agora/script/Engine.d 97.34% <0.00%> (-0.15%) :arrow_down:
source/agora/test/ValidatorRecurringEnrollment.d 95.19% <0.00%> (+0.96%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ef399df...80fb953. Read the comment docs.

hewison-chris commented 2 years ago

We should never add a preimage to a header. They indicate that we slashed a validator during consensus and that fact should never change after externalization of the slot.

Also they are part of the block/blockheader hash, so doing so would invalidate any previous signature.

What we probably should just do is not accept the signature from a slashed validator, even if we get their preimage after the block externalization.

Yes I see your point, the block should be considered immutable at this point except for the signature and validators bitmask header fields, which are not included in the hash. 🤦

linked0 commented 2 years ago

I'm running the validator built with these commits and continuously getting the error message as follows. The slashed validator is my validator anyway.

2022-04-15 06:24:14,87 Info [agora.network.Manager] - Received blocks [282..305]
2022-04-15 06:24:14,93 Error [agora.consensus.state.Ledger] - Block#282: Header is not consistent. The signature at index 23 is included for slashed validator boa1xz7hrkcjgecknn25npj5y7pkqtcw0qn48mvzddrur9clnq7zdenfjmjnpj8

I think we should re-run the Testnet.