Make Sequencer and Batcher generic over the type of log being stored #44
Conversation
|
Could you rebase on the latest main branch? You probably forked off of one of the not-yet-merged PRs, but then all the commit hashes changed when the PR was "Rebase and merge"-ed. I think it should work if you |
lukevalenta
left a comment
lukevalenta
left a comment
There was a problem hiding this comment.
Only got partway through, but I'll leave the comments so far.
I just fought git for nearly an hour and got nowhere :( Do you wanna try, or maybe just squash merge? |
…; not compiling bc wasm-bindgen doesn't like generics
…mpl Serialize so that Box<dyn PendingLogTrait> can be constructed
…ave to impl Serialize so that Box<dyn PendingLogTrait> can be constructed" This reverts commit 02ae568.
…em a monomorphization
…ithTimeStamp::sign take &[&dyn CheckpointSigner]
lukevalenta
force-pushed
the
genericize-tree
branch
from
36f592d to
f636a68
Compare
Switch some of the error types (SignerError -> NoteError) to better match intended use. Aside: the error types across packages could use some cleanup! Maybe we just export one error type per package.
Done! After the rebase there was a small diff (and I managed to overwrite author as well--we can fix before merging) that required some cleanup, and I also took the liberty to clean up some |
| } | ||
|
|
||
| /// An object that can produce a [note signature](https://github.com/C2SP/C2SP/blob/main/signed-note.md) for a given checkpoint | ||
| pub trait CheckpointSigner { |
There was a problem hiding this comment.
Why define CheckpointSigner as a separate trait instead of adding fn verifier(...) to the signed_note::Signer trait? All checkpoints are signed notes, so I think that would work out fine.
There was a problem hiding this comment.
Good q, I should have put my reasoning at the top. A plain NoteSigner is not sufficient because it only provides the implementer a raw msg: &[u8]. This is insufficient for a RFC6962 signing process, since it needs to know the timestamp, tree size, and root hash. The workaround for this rn is to separately create the signature using these values gotten from elsewhere, then make a dummy signer (InjectedSigner) that's prepopulated with the signature.
The method I'm proposing is to make a signing trait that exposes the checkpoint components to the signer. This is compatible with RFC6962 signers, tlog-cosignature signers, and plain Ed25519 signers. It also does away with InjectedSigner.
There was a problem hiding this comment.
Ah I hadn't registered the extra parameters to sign. That makes sense!
| self.metrics | ||
| .entry_count | ||
| .with_label_values(&[typ, add_leaf_result.source()]) | ||
| .with_label_values(&logging_labels) |
There was a problem hiding this comment.
The size of logging_labels has to exactly match the corresponding counter vec in metrics.rs, so that puts a restraint on the number of labels that fn logging_labels() returns. Not sure the best way to make that generic, but OK to punt on that to later.
There was a problem hiding this comment.
I'm not sure I understand. What's the issue that arises if Foo::logging_labels() returns a vec of size 3 and Bar::logging_labels() returns a vec of size 8?
There was a problem hiding this comment.
We just have to make sure that logging_labels is the same size as the corresponding predefined labels at https://github.com/cloudflare/azul/blob/main/crates/ct_worker/src/metrics.rs#L72 or we run into the error described at https://docs.rs/prometheus/latest/src/prometheus/vec.rs.html#245-246 which results in a panic in https://docs.rs/prometheus/latest/prometheus/core/struct.MetricVec.html#method.with_label_values.
- Move SequenceMetadata to tlog_tiles/src/entries.rs and use it more consistently when passing (index,timestamp) tuples around - Reuse serialize_sth_signature_input helper in sign() fn - Replace 'sequenced_entries' with 'cache_metadata' in sequence loop
lukevalenta
left a comment
lukevalenta
left a comment
There was a problem hiding this comment.
I went ahead and fixed some of the small nits I had and left a couple questions open, but otherwise this looks good!
|
very minor nit (no need to update the PR): the de-facto norm is imperative verb form for git messages, so a commit message would be "Rename x to y" instead of "Renamed x to y", so you could say "If this commit is applied, it will ": https://gist.github.com/robertpainsi/b632364184e70900af4ab688decf6f53#commit-message-guidelines |
Sorry, ya this is an old bad habit of mine |
2 participants
Sorry, this is a big PR. At the highest level,
Sequencerwas doing a lot of CT-specific actions: loading RFC6962 signing keys, logging cert info, hashing leaves in a specific way, signing precerts in a specific way, etc. This PR (hopefully) has genericized all of these operations, so thatSequencercan be instantiated over many kinds of logs. The CT functionality was moved toStaticCTSequencerand related log entry types.I think each commit is pretty coherent within itself, so I recommend going thru commit-by-commit. And ofc ask me any questions.