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

bip-0011/12 - fixed broken implementation url #1351

Merged
merged 2 commits into from
Sep 30, 2022
Merged

bip-0011/12 - fixed broken implementation url #1351

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df9c3fa
fixed bip-0011/12 broken url - implementation
russeree Aug 12, 2022
61d6631
Update BIP 11/12 OP_EVAL implementation commit url
russeree Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bip-0011.mediawiki
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ A weaker argument is OP_CHECKMULTISIG should not be used because it pops one too

OP_CHECKMULTISIG is already supported by old clients and miners as a non-standard transaction type.

https://github.com/gavinandresen/bitcoin-git/tree/op_eval
https://github.com/gavinandresen/bitcoin-git/tree/e679ec969c8b22c676ebb10bea1038f6c8f13b33
Copy link
Member

Choose a reason for hiding this comment

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

This is an old commit. Why not the most recent 77f21f1583deb89bf3fffe80fe9b181fedb1dd60?

Or perhaps link https://github.com/bitcoin/bitcoin/pull/669/files ?

Copy link
Contributor Author

@russeree russeree Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Updated Commit

Ack'd to fixing with the more modern commit.

The reason I chose the commit was it was referenced as the OP_EVAL implementation by Gavin Andresen in bip11/12 and would match the branch at the time the BIP was submitted. Though yours is newer and would represent the final state of the branch before the branch was removed.

My method to determine the location of the deleted branch was to run
git log --all --full-history -- "**op_eval*"

The result provided this as well as your commit linked.

image

In regards to the "Or perhaps".

I feel this would be the most appropriate solution for the brokens links as it was the entire merged implementation of the code changes, but isn't Gavin Andresen original 'implementation'.

Advantages to this method

  • Actual implementation into BitcoinQt is clear.
  • Preserved as a part of the bitcoin/bitcoin repo.

Disadvantages to this method

  • Doesn't wholly represent the BIP authors implementation.

Closing statement.

I would like to implement it as a link to Gavins implementation as to preserve the original BIP as much as possible.

If you would like to use the link to the OP_EVAL and merge into bitcoin/bitcoin I can do that as well. Please let me know which fix you would like and I will implement immediately.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure there was a distinction between the "original" and the final version in this sense - BIPs typically get updated as the proposal changes.

But ultimately, it's a question of what @gavinandresen will ACK here.


== Post History ==

Expand Down
2 changes: 1 addition & 1 deletion bip-0012.mediawiki
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ Example of a transaction that must fail for both old and new miners/clients:

==Reference Implementation==

https://github.com/gavinandresen/bitcoin-git/tree/op_eval
https://github.com/gavinandresen/bitcoin-git/tree/e679ec969c8b22c676ebb10bea1038f6c8f13b33

==See Also==

Expand Down