Skip to content

BIP126: Grammar fix and review#4

Merged
kristovatlas merged 2 commits intoOpenBitcoinPrivacyProject:masterfrom
dcousens:patch-1
Jul 7, 2016
Merged

BIP126: Grammar fix and review#4
kristovatlas merged 2 commits intoOpenBitcoinPrivacyProject:masterfrom
dcousens:patch-1

Conversation

@dcousens
Copy link
Copy Markdown

@dcousens dcousens commented Jun 23, 2016

ping @justusranvier, I only just saw #3, so I've partially started review here. I can move the comments there if needed.

@dcousens
Copy link
Copy Markdown
Author

dcousens commented Jun 23, 2016

When a Bitcoin transaction contains inputs that reference previous transaction outputs sent to different Bitcoin addresses

Does this BIP also apply to non-addressable output scripts?

defining standard forms for transactions containing heterogenous input scripts.

It seems so, might be worth making that consistent?

@dcousens
Copy link
Copy Markdown
Author

dcousens commented Jun 23, 2016

Find the smallest combination of inputs whose value is at least the value of the desired spend.

Is this the total desired spend? If so, what is the value of the "desired" spend in step 2?
If not (as it appears to be indicated in step 3), how is this value chosen?

@dcousens
Copy link
Copy Markdown
Author

dcousens commented Jun 23, 2016

Clients which create intentional HITs must have the capability to form alternate form HITs, and must do so for a non-zero fraction of the transactions they create.

By this do you mean a change in the procedure? Or do you mean the method by which the inputs are chosen must be non-deterministic for some portion of transactions created?
Or?

@dcousens dcousens changed the title grammar fix Grammar fix and review Jun 23, 2016
@dcousens
Copy link
Copy Markdown
Author

@kristovatlas I hope you don't mind me reviewing this here, it seemed the most coherent place. But I'd be happy to take this to email or a GitHub issue if you'd prefer.

@dcousens dcousens changed the title Grammar fix and review BIP126: Grammar fix and review Jun 23, 2016
@justusranvier
Copy link
Copy Markdown

justusranvier commented Jun 23, 2016

@dcousens

Does this BIP also apply to non-addressable output scripts?
It seems so, might be worth making that consistent?

I don't understand what you mean by either of these questions. Can you give a specific example of what you're talking about?

Is this the total desired spend? If so, what is the value of the "desired" spend in step 2?
If not (as it appears to be indicated in step 3), how is this value chosen?

Yes, this does need to be clarified.

If there is only one party forming the transaction, then then "spend output" and "change output" in step 3 are actually additional change outputs. In this step, "spend output" really means an output of a specified value. It's explained more precisely in the Rules section.

If more than party is forming the transaction, then each party gets one output of the designated size, and (probably) one change output.

By this do you mean a change in the procedure? Or do you mean the method by which the inputs are chosen must be non-deterministic for some portion of transactions created?
Or?

Mostly the procedure is written from the perspective of a normal wallet which is encountering unavoidable HITs as part of its normal operation. Those wallets will sometimes be capable of forming standard form transactions, and sometimes need to create alternate form transactions, and sometimes won't be able to follow this procedure at all.

Clients that allow multiple parties to form HITs will usually be able to create any form they want, and only rarely be constrained.

If intentional HITs always use standard form, then that means it's possible to find alternate form HITs on the blockchain and assume they are probably unintentional. To prevent this, clients that form intentional HITs should sometimes choose to make alternate form transactions even if they could have created a standard form transaction.

@kristovatlas
Copy link
Copy Markdown
Member

ACK f51a8e1

@kristovatlas
Copy link
Copy Markdown
Member

ACK 05e96d2

@kristovatlas
Copy link
Copy Markdown
Member

@dcousens thanks for the review. are you still doing additional review, or is this ready to be merged?

@kristovatlas
Copy link
Copy Markdown
Member

Also, this is a perfectly good place to post your feedback, I think -- thanks

@kristovatlas
Copy link
Copy Markdown
Member

I've ACK'd these commits, please submit a new PR for additional changes.

@kristovatlas kristovatlas merged commit da43de2 into OpenBitcoinPrivacyProject:master Jul 7, 2016
@dcousens dcousens deleted the patch-1 branch July 7, 2016 01:51
@dcousens
Copy link
Copy Markdown
Author

dcousens commented Jul 7, 2016

Thanks @kristovatlas, sorry I've been hammered at work this week.
If I find any other changes I'll make a new PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants