8000 Add methods for pushing locktimes by tcharding · Pull Request #1629 · rust-bitcoin/rust-bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add methods for pushing locktimes #1629

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Feb 10, 2023
Merged

Add methods for pushing locktimes #1629

merged 1 commit into from
Feb 10, 2023

Conversation

tcharding
Copy link
Member
@tcharding tcharding commented Feb 7, 2023 •
edited
Loading

Lock times are u32 and can require encoding using 5 bytes.

Add methods push_lock_time and push_sequence for pushing absolute lock times and sequence numbers. We do not push relative locktimes because they are only 16 bits from the original sequence number.

@tcharding tcharding marked this pull request as draft February 7, 2023 05:42
@tcharding tcharding mentioned this pull request Feb 7, 2023
Open
Kixunil
Kixunil reviewed Feb 7, 2023
View reviewed changes
@Kixunil
Copy link
Collaborator
Kixunil commented Feb 7, 2023

Regarding naming, to avoid people thinking these push opcodes as well, I suggest calling them push_sequence and push_lock_time.

@tcharding tcharding force-pushed the 02-07-push-locktimes branch from b7c5678 to 926e945 Compare February 7, 2023 07:36
@tcharding
Copy link
Member Author
tcharding commented 8000 Feb 7, 2023

Used the new names as suggested. into() doesn't work.

@apoelstra
Copy link
Member

https://doc.rust-lang.org/std/primitive.i64.html#impl-From%3Cu32%3E-for-i64 suggests that .into should work ... and should've worked since 1.5.0.

@tcharding
Copy link
Member Author 
        self.push_int(u32::from(sequence) as i64)

Works but is not any better. From<Sequence> for i64 is not implemented.

@tcharding
Copy link
Member Author 
error[E0277]: the trait bound `i64: From<Sequence>` is not satisfied
   --> bitcoin/src/blockdata/script/builder.rs:118:23
    |
118 |         self.push_int(sequence.into())
    |                       ^^^^^^^^ ---- required by a bound introduced by this call
    |                       |
    |                       the trait `From<Sequence>` is not implemented for `i64`
    |
    = help: the following other types implement trait `From<T>`:
              <f32 as From<i16>>
              <f32 as From<i8>>
              <f32 as From<u16>>
              <f32 as From<u8>>
              <f64 as From<f32>>
              <f64 as From<i16>>
              <f64 as From<i32>>
              <f64 as From<i8>>
            and 74 others
    = note: required for `Sequence` to implement `Into<i64>`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `bitcoin` due to previous error

@apoelstra
Copy link
Member

sequence.to_consensus_u32().into() should work, and is better than casting (which is clearly fine in this case, but is a code smell we'd like to eliminate)

@tcharding
Copy link
Member Author

Ha ha, I'm a wombat, the whole time it was the as u64 that was the problem and I was looking at the to_consensus_u32 - talk about tunnel vision :)

@tcharding tcharding force-pushed the 02-07-push-locktimes branch from 926e945 to e106f4f Compare February 8, 2023 21:07
@tcharding
Copy link
Member Author

Removed usage of as u64.

apoelstra
apoelstra previously approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e106f4f29d47ce12299dff7a6c9476355b1f3e58

Kixunil
Kixunil previously approved these changes Feb 9, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e106f4f29d47ce12299dff7a6c9476355b1f3e58

@apoelstra apoelstra mentioned this pull request Feb 9, 2023
Merged
@tcharding
Add push_* methods for lock times
1e0e712 
Lock times are u32 and can necessitate encoding using 5 bytes. As such
they are "special".

Add methods `push_lock_time` and `push_sequence` for pushing absolute
lock times and sequence numbers. We do not push relative locktimes
because they are only 16 bits from the original sequence number.
@tcharding tcharding dismissed stale reviews from Kixunil and apoelstra via 1e0e712 February 10, 2023 01:23
@tcharding tcharding force-pushed the 02-07-push-locktimes branch from e106f4f to 1e0e712 Compare February 10, 2023 01:23
@tcharding tcharding marked this pull request as ready for review February 10, 2023 01:24
@tcharding
Copy link
Member Author

No changes since acks, just rebased to pick up a recently merged commit and re-wrote the PR description.

Kixunil
Kixunil approved these changes Feb 10, 2023
View reviewed changes
Copy link
Collaborator
@Kixunil Kixunil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1e0e712

apoelstra
apoelstra approved these changes Feb 10, 2023
View reviewed changes
Copy link
Member
@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1e0e712

@apoelstra apoelstra merged commit 1514205 into rust-bitcoin:master Feb 10, 2023
@tcharding tcharding deleted the 02-07-push-locktimes branch February 13, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kixunil Kixunil Kixunil approved these changes

@apoelstra apoelstra apoelstra approved these changes

Assignees
No one assigned
Labels
release notes mention
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tcharding @Kixunil @apoelstra
0