polkadot-spec icon indicating copy to clipboard operation
polkadot-spec copied to clipboard

Incoperate recommendations from Gossamer Audit

Open FlorianFranzen opened this issue 3 years ago • 2 comments

There have been a lot of good recommendations in the gossamer audit. I will make issues out of them, so that we can update our spec accordingly.

FlorianFranzen avatar Mar 25 '22 18:03 FlorianFranzen

The recommendations:

  • [x] The spec should be explicit regarding how the decoding operation should handle situations where extra data is at the end of some SCALE encoded blob. That is, whether decoding is lenient and returns without an error, or strict and returns an error.

  • [x] The spec, in particular section 2.1 makes no mention of whether the Storage Trie should allow empty values and empty keys, and how they should be handled. In particular Definition 2.4 doesn't clarify whether storage_set(k, "") and storage_clear(k) are equivalent operations. The specification appears to imply that empty keys and values are allowed, and both Gossamer and the Substrate reference implementation abide by this, but an explicit definition is preferred.

  • [x] Section 6.1.1, Remark 6.2 states that "In Polkadot, all authorities have the weight w_A = 1", but the threshold in Definition 6.12 simply uses the number of authorities instead of the sum of their weights. In this regard, Gossamer follows the spec but Substrate reads the weights from the runtime. The main impact is that this could cause a consensus split should a runtime upgrade set authority weights other than 1.

  • [x] Definition 6.12 is insufficient in clarifying how these non-integer computations should be performed - Doesn't specify floating points or how it should be implemented - so rounding and inconsistent results might cause a consensus split (i.e. when the rounding occurs) - is this floating point? fixed decimal? etc

  • [x] The VRF section A.4 is empty, though this appears to be tracked in https://github.com/w3f/polkadot-spec/issues/467

  • [x] Section 2.1 states: "There is no assumption either on the size of the key nor on the size of the data stored under them, besides the fact that they are byte arrays with specific upper limits on their length. The limit is imposed by the encoding algorithms to store the key and the value in the storage trie". For clarity, consider explicitly specifying the associated length limits or link to a section that does.

lamafab avatar May 04 '22 11:05 lamafab

@lamafab What is left to do here?

FlorianFranzen avatar Aug 11 '22 08:08 FlorianFranzen

Definition 6.12 is insufficient in clarifying how these non-integer computations should be performed - Doesn't specify floating points or how it should be implemented - so rounding and inconsistent results might cause a consensus split (i.e. when the rounding occurs) - is this floating point? fixed decimal? etc

"Hot-fix" in #529

lamafab avatar Aug 24 '22 15:08 lamafab