signer: track sequence numbers

[irriden]

restarts if signer is initialized, AND (broker restarts or broker skips sequence

[Evanfeenstra]

• we should avoid pulling in esp_idf_sys dependencies here. It should be generalizable enough to handle different architectures. Maybe we return a specific anyhow Err message here, that we can check in the caller (the esp32 or other) to restart if needed?

• isn't sequence always equal to zero on the very first message? Which would cause a restart on the first message/

• if counter is equal to u16::MAX, why are you storing the sequence here? Does that decrement it by one? fetch_add wraps around back to zero after hitting the max, right?

[Evanfeenstra]

ok, just to review:

• move the AtomicU16 out to the caller (sphinx-key)
• pass an expected_sequence Option<u16> as an argument into the root handle
• make a custom error using thiserror crate so we can match a BadSequence error and restart the ESP32 in that case

if there wasn't an error than we know the sequence was incremented by one. So we dont need to return any new field from the tuple returned from .handle

[irriden]

@Evanfeenstra what should we do in the case where Option is None again?

[Evanfeenstra]

@Evanfeenstra what should we do in the case where Option is None again?

Simply not check the sequence. In case we make an SDK and someone doesnt care about multi-signer

[irriden]

Ok - but for the multi signer case, we need to somehow return the sequence received from the broker - and use that as the first reference point.

We could do a Option<&mut u16> ?

[irriden]

I thought we could use the info returned in BadSequence but that stops further execution.

[Evanfeenstra]

Ok - but for the multi signer case, we need to somehow return the sequence received from the broker - and use that as the first reference point.

We could do a Option<&mut u16> ?

Just u16. Always returning the sequence number from the broker

[irriden]

@Evanfeenstra the Error now looks like this.

My concern is that these #[from] conversions require std::error::Error to be implemented, and that's of course gated by std in our dependencies.

#[derive(Error, Debug)]
pub enum VlsHandlerError {
    #[error("vls protocol error: {0}")]
    VlsProtocol(#[from] vls_protocol::Error),
    #[error("vls signer error: {0}")]
    VlsSigner(#[from] vls_protocol_signer::handler::Error),
    #[error("failed lss_msg.to_vec: {0}")]
    LssWrite(#[from] anyhow::Error),
    #[error("invalid sequence: {0}, expected {1}")]
    BadSequence(u16, u16),
}

[Evanfeenstra]

@Evanfeenstra the Error now looks like this.

My concern is that these #[from] conversions require std::error::Error to be implemented, and that's of course gated by std in our dependencies.

#[derive(Error, Debug)]
pub enum VlsHandlerError {
    #[error("vls protocol error: {0}")]
    VlsProtocol(#[from] vls_protocol::Error),
    #[error("vls signer error: {0}")]
    VlsSigner(#[from] vls_protocol_signer::handler::Error),
    #[error("failed lss_msg.to_vec: {0}")]
    LssWrite(#[from] anyhow::Error),
    #[error("invalid sequence: {0}, expected {1}")]
    BadSequence(u16, u16),
}

Hmm ok i see. Well i guess we just need to use map_err then. Sorry for adding confusion

[irriden]

@Evanfeenstra the Error now looks like this. My concern is that these #[from] conversions require std::error::Error to be implemented, and that's of course gated by std in our dependencies.

#[derive(Error, Debug)]
pub enum VlsHandlerError {
    #[error("vls protocol error: {0}")]
    VlsProtocol(#[from] vls_protocol::Error),
    #[error("vls signer error: {0}")]
    VlsSigner(#[from] vls_protocol_signer::handler::Error),
    #[error("failed lss_msg.to_vec: {0}")]
    LssWrite(#[from] anyhow::Error),
    #[error("invalid sequence: {0}, expected {1}")]
    BadSequence(u16, u16),
}

Hmm ok i see. Well i guess we just need to use map_err then. Sorry for adding confusion

No problem at all - all good learning :)