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

Make sequence a newtype #1082

Closed
Kixunil opened this issue Jun 30, 2022 · 7 comments
Closed

Make sequence a newtype #1082

Kixunil opened this issue Jun 30, 2022 · 7 comments
Labels
1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems good first issue

Comments

@Kixunil
Copy link
Collaborator

Kixunil commented Jun 30, 2022

Making sequence a simple newtype could avoid accidentally mixing it up with other things. It looks like there's such a bug in miniscript but regardless of whether that one is a bug newtypes are great in security-critical code like Bitcoin.

Additionally, we can have a bunch of handy methods and constants such as is_final(), is_rbf()...

Proposed declaration:

#[derive(Debug, Copy, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)]
pub struct Sequence(u32); // perhaps the field could be `pub`?

// impl Display for Sequence
// Any interesting (math) operators? Seems too much to me but maybe someone has a use case

The type is a bit weird because u32::MAX means infinity but it's probably OK or Ord/Eq and we can be extra careful for other operations.

@Kixunil Kixunil added API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems brainstorm 1.0 Issues and PRs required or helping to stabilize the API labels Jun 30, 2022
@apoelstra
Copy link
Member

I think we should make the field pub. concept ACK for sure, it would greatly simplify miniscript's older/after logic (or at least, simplify code review)

@dr-orlovsky
Copy link
Collaborator

@Kixunil if you are interested, that's how I did SeqNo type some time ago: https://docs.rs/descriptors/0.8.0/src/descriptors/locks.rs.html#96

@tcharding
Copy link
Member

Warning to anyone reading the good-first-isue, this is very likely not a trivial issue to do because nSequence is fraught with corner cases and this involves API design, which will mean the PR that solves this issue will likely require a lot of review before it gets merged. Its a good-first-issue in that you will really have to learn about timelocks in bitcoin to do this one though :) I have this queued up in my todo list after #994 gets done but any one is free to jump in.

@nlanson
Copy link
Contributor

nlanson commented Jul 10, 2022

@tcharding If you don't mind I can get this done within the next day or so.

Happy for merge to be after #994 so I do the rebasing.

@Kixunil
Copy link
Collaborator Author

Kixunil commented Jul 10, 2022

@tcharding I think it largely depends on how well one knows Rust. This is the kind of newtype that's very easy. Review will resolve any issues and if my review will help someone learn Rust&Bitcoin better I'll happily do it. :)

@tcharding
Copy link
Member

tcharding commented Jul 11, 2022

Go for it @nlanson, don't worry about merge order in relation to rebasing, I was waiting for #994 because I thought many of the same concepts (and problems) would come up while doing nLockTime (its the less complicated of the two Bitcoin time locks). But by all means go for it, there is only an issue with Ord to be resolved in #994 (hopefully). Cheers!

apoelstra added a commit that referenced this issue Jul 17, 2022
e34bc53 Add new type for sequence (Noah Lanson)

Pull request description:

  #1082

  Created a new type for txin sequence field with methods to create sequences with relative time locks from block height or time units.

ACKs for top commit:
  Kixunil:
    ACK e34bc53
  tcharding:
    ACK e34bc53
  apoelstra:
    ACK e34bc53

Tree-SHA512: 6605349d0312cc36ef9a4632f954e59265b3ba5cfd437aa88a37672fe479688aa4a3eff474902f8cc55848efe55caf3f09f321b3a62417842bfc3ec365c40688
@nlanson
Copy link
Contributor

nlanson commented Jul 21, 2022

Closed by #1093

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0 Issues and PRs required or helping to stabilize the API API break This PR requires a version bump for the next release code quality Makes code easier to understand and less likely to lead to problems good first issue
Projects
None yet
Development

No branches or pull requests

5 participants