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.

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

redefine "pegout" to better match elementsd RPC #17

Merged
merged 3 commits into from
May 20, 2019
Merged

redefine "pegout" to better match elementsd RPC #17

merged 3 commits into from
May 20, 2019

Conversation

apoelstra
Copy link
Member

No description provided.

@apoelstra apoelstra force-pushed the 2019-04-pegout-redefinition branch from 7e33918 to a24b19b Compare April 25, 2019 15:22
@stevenroose
Copy link
Member

LGTM. I built hal-elements with this.
hal elements tx decode for the second test vector tx panic'ed with Execution failed: internal error: entered unreachable code while with this PR, it shows a pegout-free transaction response.

@apoelstra apoelstra force-pushed the 2019-04-pegout-redefinition branch from a24b19b to 3e13349 Compare April 25, 2019 15:40
@apoelstra
Copy link
Member Author

Rebased; changed version to 0.7.1 rather than 0.6.1

@apoelstra apoelstra force-pushed the 2019-04-pegout-redefinition branch 2 times, most recently from 1ef2ad0 to 1c92c2a Compare April 25, 2019 16:07
@roconnor-blockstream
Copy link

LGTM

@apoelstra apoelstra mentioned this pull request Apr 25, 2019
Closed
@instagibbs
Copy link

Appears to sync with elementsd. utACK

stevenroose
stevenroose approved these changes Apr 25, 2019
View reviewed changes
Copy link
Member

@stevenroose stevenroose left a comment

Choose a reason for hiding this comment

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

re-ACK 1c92c2a
working with hal-elements too

@roconnor-blockstream
Copy link

See also ElementsProject/elements#598 (comment)

@apoelstra apoelstra force-pushed the 2019-04-pegout-redefinition branch from 1c92c2a to 4d7ad73 Compare May 14, 2019 19:57
@apoelstra
Copy link
Member Author

Rebased.

@apoelstra
Copy link
Member Author

Looks good I believe with ElementsProject/elements#610 both rust-elements and elements should agree.

@apoelstra apoelstra merged commit ae548a4 into master May 20, 2019
@apoelstra apoelstra deleted the 2019-04-pegout-redefinition branch May 20, 2019 15:48
roconnor-blockstream
roconnor-blockstream reviewed May 21, 2019
View reviewed changes
src/transaction.rs
@@ -400,23 +404,37 @@ impl TxOut {

// Parse destination scriptpubkey
let script_pubkey = if let Some(Instruction::PushBytes(data)) = iter.next() {
Script::from(data.to_owned())
if data.is_empty() {
Copy link

Choose a reason for hiding this comment

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

Elements was modified to make 0 byte pushes legal @ https://github.com/ElementsProject/elements/pull/610/files#diff-f7ca24fb80ddba0f291cb66344ca6fcbR222

So this block needs to be reverted now.

Copy link

Choose a reason for hiding this comment

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

not merged yet! but looks like ACKs from relevant parties

Copy link

Choose a reason for hiding this comment

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

oh indeed.

Copy link

Choose a reason for hiding this comment

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

610 is now merged and this needs to be reverted now.

Copy link
Member Author

Choose a reason for hiding this comment

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

#20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roconnor-blockstream roconnor-blockstream roconnor-blockstream left review comments

@instagibbs instagibbs instagibbs left review comments

@stevenroose stevenroose stevenroose approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apoelstra @stevenroose @roconnor-blockstream @instagibbs