Dynamic signature sampling

[doubledup]

This counts the number of times each validator's signature is used in the current session. It then uses the recommendation from W3F to adjust the required number of signatures for a commitment to be accepted.

@vgeddes For now, I've stored these counts by validator address only (not session id) as I'm not yet sure of a use for the counts from past sessions. If we need them, I'll extend the key to include session id.

If we can store them by validator index, there's a potential optimisation where we can pack multiple counts into a single word, eg. keep a uint256[] where each count is a uint16 packed into an entry in that array - similar to how we work with bitfields. We'll have at most 2**16-1 = 65 535 uses of each validator's signature in submitInitial calls. For reference, with 6s block time & 24h sessions, we'll see 24*60*60/6 = 14 400 blocks each session.

Resolves: SNO-671, SNO-672, SNO-673, SNO-674, SNO-634

[yrong]

@doubledup I remember the initial suggestion from Web3 team is Limiting validators reuse per relayer(also mentioned in the Oak audit report), right? Just curious what's the benefit of this solution over the previous one or why they abandon that one?

There is a draft PR #879 for that though I closed later. I'm not sure but my intuition will the current implementation cost much more gas(considering an invariant length array validatorsUsedThisSession requires deletion in each session and some logarithmic arithmetic introduced)?

[vgeddes]

@doubledup I remember the initial suggestion from Web3 team is Limiting validators reuse per relayer(also mentioned in the Oak audit report), right? Just curious what's the benefit of this solution over the previous one or why they abandon that one?

There is a draft PR #879 for that though I closed later. I'm not sure but my intuition will the current implementation cost much more gas(considering an invariant length array validatorsUsedThisSession requires deletion in each session and some logarithmic arithmetic introduced)?

@ron after a call with Web3 (I can't remember if you were on the call), we decided to go with this solution, where the number of checked signatures dynamically increases. The benefit of this approach is that relayers don't need to put up deposits.

[doubledup]

@yrong For gas costs, we can potentially reduce them by packing multiple counts into an array like uint256[]. This requires a stable ordering of the validators within a session though - something I'm not yet sure we can count on.

The log arithmetic is already fairly performant as we're rounding to an integer rather than calculating a fraction: https://github.com/Snowfork/snowbridge/pull/956/files#diff-473fead6cd500a4a05f06d0cea4d9ef166c9bdbe2a2e1a06a33d2e1aedcc428eR427-R433

[yrong]

For now, I've stored these counts by validator address only (not session id) as I'm not yet sure of a use for the counts from past sessions. If we need them, I'll extend the key to include session id.

Yes, I prefer to extend the key to include session id and would suspect we can merge the two storages signatureCounters and validatorsUsedThisSession into one.

Moreover, I would doubt even necessary for the auxiliary storage validatorsUsedThisSession which used only for cleanup purposes? Just curious Is there still a reward with gas refund for freeing up storage space? (vaguely recall it's canceled in some EIP but I can't find it now).

[vgeddes]

I suggest a different data structure for the usage counters, one that has efficient space complexity and can be deleted in constant time.

Since we know we need to keep counters for both the current and next session, we can update the ValidatorSet struct with a counters field as follows:

    struct ValidatorSet {
        uint128 id;
        uint128 length;
        bytes32 root;
        uint256[] counters;
    }

This counters field is a packed array of uint16 usage counters for each validator. Updating the packed array can be performed using bitfield math. Deleting it is a simple delete currentValidatorSet.counters 1-liner.

[vgeddes]

I suggest a different data structure for the usage counters, one that has efficient space complexity and can be deleted in constant time.

Oops I see that you suggested mostly the same thing in the PR description 👍

However, if we make sure that the packed array is scoped to a single session, then it is possible to track validators by their leaf index, as they will remain static during a session.

As I said previously we need to have a packed array for both the current and next session.

On session handover, we also need to copy over the counters from the next session to the current session, i.e:

copyCounters(from=nextValidatorSet.counters, to=currentValidatorSet.counters);

and then we reset the counters for the next session to zero and to match the size of the new next validator set.

I did some gas tests with this approach and it doesn't look too bad honestly:

| test/PackedArrayGasTest.t.sol:ArrayThing contract |                 |        |        |        |         |
|---------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost                                   | Deployment Size |        |        |        |         |
| 298544                                            | 1519            |        |        |        |         |
| Function Name                                     | min             | avg    | median | max    | # calls |
| copyCounters                                      | 57593           | 57593  | 57593  | 57593  | 1       |
| deleteCountersA                                   | 7920            | 7920   | 7920   | 7920   | 1       |
| initCountersA                                     | 168104          | 168104 | 168104 | 168104 | 1       |
| initCountersB                                     | 168047          | 168047 | 168047 | 168047 | 1       |

Test Script (assuming 1000 validators):

// SPDX-License-Identifier: Apache-2.0
pragma solidity ^0.8.20;

import "forge-std/Test.sol";
import "forge-std/console.sol";

import {Agent} from "../src/Agent.sol";

contract ArrayThing {

    uint256 constant NUM_VALIDATORS = 1024;
    uint256 constant ELEMENT_SIZE = 2;
    uint256 constant CONTAINER_SIZE = 32;
    uint256 constant NUM_CONTAINERS = (NUM_VALIDATORS * ELEMENT_SIZE) / CONTAINER_SIZE;

    uint256[] public countersA;
    uint256[] public countersB;

    function initCountersA() public {
        countersA = new uint256[](NUM_CONTAINERS);
    }

    function initCountersB() public {
        countersB = new uint256[](NUM_CONTAINERS);
    }

    function deleteCountersA() public {
        delete countersA;
    }

    function copyCounters() public {
        for (uint i = 0; i < countersB.length; i++) {
            countersA[i] = countersB[i];
        }
    }
}

contract PackedArrayGasTest is Test {

    ArrayThing public thing;

    function setUp() public {
        thing = new ArrayThing();
    }

    function testCounters() public {
        thing.initCountersA();
        thing.initCountersB();
        thing.copyCounters();
        thing.deleteCountersA();
    }
}

[yrong]

@doubledup I've updated Readme with required steps to regenerate beefy test fixtures. Hope that's helpful.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c05de23) 77.81% compared to head (b3b383b) 76.73%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #956      +/-   ##
==========================================
- Coverage   77.81%   76.73%   -1.09%     
==========================================
  Files          46       11      -35     
  Lines        1893      361    -1532     
  Branches       76       71       -5     
==========================================
- Hits         1473      277    -1196     
+ Misses        401       68     -333     
+ Partials       19       16       -3     

Flag	Coverage Δ
rust	?
solidity	76.73% <100.00%> (+5.14%)	⬆️

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

Files	Coverage Δ
contracts/src/BeefyClient.sol	87.70% <100.00%> (+15.20%)	⬆️

... and 35 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[claravanstaden]

This is pretty cool! ✨

[alistair-singh]

Looks good so far. Left some comments.

[vgeddes]

Looks good, added some minor comments.

[vgeddes]

One more thing @alistair-singh, we also need to update the documentation at https://docs.snowbridge.network/architecture/verification/polkadot to describe the design of the dynamic signature sampling algorithm.

Can either edit the markdown in this PR, or via the online gitbook editor. Either way let's make sure this task is tracked.

[yrong]

Another change we may need is for relay logic to not always pick from the first validator

snowbridge/relayer/relays/beefy/ethereum-writer.go

Lines 156 to 158 in ba4e801

	// Pick first validator who signs beefy commitment
	chosenValidator := signedValidators[0].Int64()
	msg, err := task.MakeSubmitInitialParams(chosenValidator, initialBitfield)

Instead, it makes more sense to pick from the validator set randomly.

[alistair-singh]

Instead, it makes more sense to pick from the validator set randomly.

Addressed in 2494471

[yrong]

Awesome!

[vgeddes]

Lets merge it!