Conversation
sergerad
added
the
no changelog
sergerad
requested review from
Mirko-von-Leipzig,
bobbinth and
mmagician
and removed request for
bobbinth
Mirko-von-Leipzig
left a comment
Mirko-von-Leipzig
left a comment
There was a problem hiding this comment.
Minor nits, but I think we need to allow for a single block fork.
sergerad
force-pushed
the
sergerad-validator-chain
branch
from
8586d1d to
e2dee35
Compare
| CREATE INDEX idx_validated_transactions_block_num ON validated_transactions(block_num); | ||
|
|
||
| CREATE TABLE block_headers ( | ||
| block_num INTEGER PRIMARY KEY, |
There was a problem hiding this comment.
If we also store block_commitment can we avoid pulling in the entire header each check?
There was a problem hiding this comment.
That depends on whether we want to do any additional checks on top of the block number + commitment ones I have now.
@bobbinth may have had some ideas on that point - whether there is more to compare with a new / proposed chain tip (not signed) against the previous signed chain tip beyond just block number and commitment continuity.
| &data_directory, | ||
| &accounts_directory, | ||
| &data_directory, |
There was a problem hiding this comment.
We're passing the same data_directory twice - I assume this means that we can't distinguish the two directories in bundled mode?
There was a problem hiding this comment.
Yea no need to distinguish. TBH I don't think we ever needed bundled bootstrap subcommand at all. And we are working on removing bundled command altogether in #1786.
| genesis_block_directory: PathBuf, | ||
| /// Directory to write the account secret files (.mac) to. | ||
| #[arg(long, value_name = "DIR")] | ||
| accounts_directory: PathBuf, | ||
| /// Directory in which to store the validator's database. | ||
| #[arg(long, env = ENV_DATA_DIRECTORY, value_name = "DIR")] | ||
| data_directory: PathBuf, |
There was a problem hiding this comment.
This might be for a separate discussion/PR, but I find the argument names a little misleading: it's not immediately clear whether these are input directories or output directories, for example:
genesis_block_directoryis an output location, butgenesis_config_fileis an input location (here, path, not a directory, but the same argument applies)
There was a problem hiding this comment.
Wouldn't hurt to specify _output_ for the directories I guess
| /// 3. The block is either: a. The valid next block in the chain (sequential block number, matching | ||
| /// previous block commitment), or b. A replacement block at the same height as the current chain | ||
| /// tip, validated against the previous block header. |
There was a problem hiding this comment.
what are the situations where we would run into scenario b) ?
There was a problem hiding this comment.
The order of operations for block N+1 is roughly
- [node] block is built
- [validator] block is signed
- block is signed
- block is now in validator database as latest
- signed block is returned to [node]
- [node] block is stored (and is now considered committed)
So there are several steps between (2ii) and (3) where things can go wrong e.g. node or validator crash, or network IO issue, or node database issue, etc, where the validator would have the signed block N+1 but the node is still on N.
The validator can only consider N+1 finalized once the node requests N+2. The alternative would be a two-phase ack node submit -> validator sign -> node ack. But that's more complicated than this.
3 participants
Context
Closes #1773.
In #1764 we are adding a validator subcommand to construct and sign the genesis block. In this PR, we are building on top of that and storing the genesis block as the initial chain tip in the validator DB.
The validator is updated to maintain a chain tip as it continues to sign blocks. It uses this state to validate chain continuity (block number sequence, commitment references, etc).
Changes
block_headerstable to the validator database.--data-directoryargument).validate_blocknow enforces chain continuity: sequential block numbers and matching previous block commitments.validate_blockallows for the current chain tip inblock_headerstable to be overwritten to allow for idempotence.dbmodule public so the bootstrap command can callsave_chain_tipandloaddirectly.