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

Already on GitHub? Sign in to your account

Reword description of transaction execution #816

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

gwillen commented Apr 10, 2015

I believe that the current description of transaction execution is not accurately describing how the stack machine works. (It's close but uses slightly misleading wording.) While data values are pushed when executed in the script, other opcodes are never pushed; they are simply executed.

Reword description of transaction execution
I believe that the current description of transaction execution is not accurately describing how the stack machine works. (It's close but uses slightly misleading wording.) While data values are pushed when executed in the script, other opcodes are never pushed; they are simply executed.

@apoelstra apoelstra commented on an outdated diff Apr 10, 2015

_includes/guide_transactions.md
This creates a hash of Bob's public key.
* Alice's pubkey script then pushes the pubkey hash that Bob gave her for the
first transaction. At this point, there should be two copies of Bob's
pubkey hash at the top of the stack.
-* Now it gets interesting: Alice's pubkey script adds `OP_EQUALVERIFY` to the
- stack. `OP_EQUALVERIFY` expands to `OP_EQUAL` and `OP_VERIFY` (not shown).
+* Now it gets interesting: Alice's pubkey script executes `OP_EQUALVERIFY`.
+ `OP_EQUALVERIFY` is like executing `OP_EQUAL` followed by `OP_VERIFY` (not shown).
@apoelstra

apoelstra Apr 10, 2015

Can you change "is like" to "is equivalent to"?

@apoelstra apoelstra commented on an outdated diff Apr 10, 2015

_includes/guide_transactions.md
This creates a hash of Bob's public key.
* Alice's pubkey script then pushes the pubkey hash that Bob gave her for the
first transaction. At this point, there should be two copies of Bob's
pubkey hash at the top of the stack.
-* Now it gets interesting: Alice's pubkey script adds `OP_EQUALVERIFY` to the
- stack. `OP_EQUALVERIFY` expands to `OP_EQUAL` and `OP_VERIFY` (not shown).
+* Now it gets interesting: Alice's pubkey script executes `OP_EQUALVERIFY`.
+ `OP_EQUALVERIFY` is like executing `OP_EQUAL` followed by `OP_VERIFY` (not shown).
`OP_EQUAL` (not shown) checks the two values below it; in this
@apoelstra

apoelstra Apr 10, 2015

"two values below it" doesn't make sense now that we're not talk as though opcodes are on the stack; this should be changed to "two values at the top of the stack".

@apoelstra apoelstra commented on an outdated diff Apr 10, 2015

_includes/guide_transactions.md
`OP_EQUAL` (not shown) checks the two values below it; in this
case, it checks whether the pubkey hash generated from the full
public key Bob provided equals the pubkey hash Alice provided when
- she created transaction #1. `OP_EQUAL` then replaces itself and
- the two values it compared with the result of that comparison:
+ she created transaction #1. `OP_EQUAL` pops (removes from the stack)
+ the two values it compared, and replaces them with the result of that comparison:
@apoelstra

apoelstra Apr 10, 2015

"removes from the stack" → "removes from the top of the stack"

Other than the minor comments I added inline, I strongly encourage accepting this change; it gives a much clearer picture of the stack machine operation.

fixup: Improve wording
Improve wording per apoelstra's comments.
Contributor

gwillen commented Apr 10, 2015

Thanks @apoelstra, made the suggested changes.

This probably wants to be squashed with rebase into a single commit (I made the edits through the github web editor which can't automatically do that.) I can pull to a real git client and do that myself if requested.

Thanks @gwillen, with these changes I support this PR.

maaku commented Apr 10, 2015

ACK. This is much clearer (and correct). Thanks for catching this @gwillen .

Contributor

harding commented Apr 10, 2015

This LGTM. I'll squash it down to one commit and merge it as soon as I fix the unrelated problem that's causing the CI to fail. Thanks for doing this @gwillen, and @apoelstra and @maaku for comments!

@gwillen out of curiosity, did you use the Edit links from on the documentation pages? I added those a few months ago and have been surprised at how rarely they're used.

@harding harding closed this in 3251995 Apr 10, 2015

Contributor

gwillen commented Apr 10, 2015

@harding Yes, I did use the Edit link! Other than that I wouldn't have realized it was editable. And the git web-edit workflow is surprisingly good (I've never used it before.) It really reduces the friction for stuff like this.

Thanks!

@gwillen gwillen deleted the gwillen:patch-1 branch Apr 10, 2015

Contributor

saivann commented Apr 10, 2015

@gwillen Nice to hear, thanks for your help!

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