Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a filter for TimestampCommitment and BlockHashCommitment #104

Closed
thobson88 opened this issue Jul 20, 2023 · 4 comments
Closed

Add a filter for TimestampCommitment and BlockHashCommitment #104

thobson88 opened this issue Jul 20, 2023 · 4 comments

Comments

@thobson88
Copy link
Collaborator

thobson88 commented Jul 20, 2023

The TrivialCommitment trait includes (optional) support for filtering of the candidate data.

In the case of a TimestampCommitment, the candidate data is a Bitcoin block header. Currently the expected data (a Unix timestamp) is sought inside the whole header, not just the timestamp field. This introduces a possible attack vector. An attacker could produce a header with a different timestamp but which contains, in a different field, the 32 bits matching a different timestamp. Without filtering of the candidate data, this bogus header would be accepted as valid (in the sense that the commitment would verify successfully).

The same applies to the BlockHashCommitment (where we should filter on the Merkle root field).

@thobson88
Copy link
Collaborator Author

thobson88 commented Jul 21, 2023

For this:

  • make TimestampCommitment into a trait in trustchain-core, extending the Commitment trait
    • include a default implementation of the timestamp() method
    • also include the new() method signature in the trait (with expected_data argument of type Timestamp) can't be done in a trait (as methods must have &self argument).
  • implement the trait in trustchain-ion (I've named this is BlockTimestampCommitment in trustchain-ion, commitment.rs)
  • include an implementation of the filter() method that filters out everything from the commitment content (a Bitcoin block header) except the timestamp field.
  • also implement filter() in the BlockHashCommitment.

@thobson88
Copy link
Collaborator Author

Ok, this seems to work but the default implementation of the timestamp() method inside the TimestampCommitment trait is brittle:

/// A Commitment whose expected data is a Unix time.
pub trait TimestampCommitment : Commitment {

    /// Gets the timestamp as a Unix time.
    fn timestamp(&self) -> Timestamp {
        self.expected_data()
            .as_u64()
            .unwrap()
            .try_into()
            .expect("Construction guarantees u32.")
    }
}

It would be ok if we could enforce the condition that the expected_data must have type Timestamp, but I can't see a way to do that.

Also, I haven't yet run the implementation tests.

On the plus side, the implementation of verifiable_timestamp in IONVerifier is now a bit simpler, and the filter() is implemented.

@thobson88 thobson88 changed the title Add a filter for TimestampCommitment and MerkleRootCommitment Add a filter for TimestampCommitment and BlockHashCommitment Jul 21, 2023
@sgreenbury
Copy link
Collaborator

Nice one, this is much better and fixes the issue!

I've made a couple of modifications:

  • Fixed test_block_hash_commitment() given revision and added failing case to test_block_timestamp_commitment() (2f7902c)
  • Changed TrivialCommitment and Commitment traits to take a generic (that defaults to Value) for the expected_data (556f122). We can then implement these traits with the generic as a Timestamp for the BlockTimestampCommitment (e.g. here). The verify_content() method has an additional json!() on the expected data so it is always a Value.

I think this is good to go in #106 but some potential options for future:

  • Refactoring BlockHashCommitment to handle committing to both the timestamp and PoW hash as we can implement both Commitment<Value> and Commitment<Timestamp> (distinct generics)
  • We could refactor the commitment traits to have an associated type to make the candidate data specific relevant types instead of Value e.g.:
    trait Commitment {
      type Data;
      // ...
      fn decode_candidate_data(&self) -> fn(&[u8]) -> CommitmentResult<Data>;
      // ...
    }
    A downside of this though would be I don't think default implementations would be possible.

sgreenbury added a commit that referenced this issue Jul 27, 2023
…mp-commitment

Add filtering for block hash and timestamp commitments, add generic expected data (#104)
sgreenbury added a commit that referenced this issue Aug 11, 2023
…ifier

- Type state pattern for `Verifier` with `FullClient` and `LightClient` (#101)
- Refactor of `VerifiableTimestamp` (#102)
- Filtering for block commitments and revised commitment traits with generics (#104)
@thobson88
Copy link
Collaborator Author

Done in #108

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants