BIP68: Simplify language and update for current implementation #245

btcdrak commented at 11:06 AM on November 20, 2015: contributor

Update according to bitcoin/bitcoin#6312 implementation

BIP68: Simplify language and update for current implementation b5bc89a67c

Edits from kinoshitajona 2bc1979601

morcos commented at 3:23 PM on November 20, 2015: member

I took a quick skim, the example code in Compatibility is still referring to the old specification.

My biggest outstanding concern is still with this flag at (1<<22). I'd like to make sure a few other people take a look at it and agree it makes sense. I think the one justification @maaku had for this is it would allow alternative coins/chains with faster block times to have finer granularity than 512 seconds and work out of the box. I'm not sure how much weight should be given to that, but on the other hand, it may be bike shedding at this point as to where exactly we put the flag and that may be a perfectly reasonable choice. So concept ACK from me.

btcdrak force-pushed on Nov 20, 2015

btcdrak commented at 5:38 PM on November 20, 2015: contributor

@morcos Thanks for taking a look. The compatibility section has been updated now. I wasnt sure if I should mention the granularity issue in relation to shorter block intervals and sidechains because it is not directly related to Bitcoin but dont have a problem adding something it is deemed important.

Overall I have tried to explain rationale as much as possible and make the BIP clearer.

The main rationale about (1<<22) I believe was taking into consideration 3 byte pushdata when using an opcode like OP_CSV. There is a paragraph to explain this. This comes from the discussion here http://bitcoinstats.com/irc/bitcoin-dev/logs/2015/10/15#l1444928045.0

And updated in this BIP to read

"Note (1 << 22) was chosen as the flag bit because it's the highest bit of a 3 byte pushdata which is a signed integer when used in conjunction with OP_CHECKSEQUENCEVERIFY; specifically 3 bytes consisting of 1 bit as the sign, 1 bit for the relative lock-time type flag, and 22 bits for the relative lock-time encoding."

jmcorgan commented at 5:54 PM on November 20, 2015: contributor

utACK

clarify specification further 7d7083f722

Update compatibility section c141645a1f

Add note about free bits and correct deployment recommendations 060c37f6d5

in bip-0068.mediawiki:None in 0500f87a26 outdated

18 | ==Motivation==
19 |
20 | -Bitcoin has sequence number fields for each input of a transaction. The original idea appears to have been that the highest sequence number should dominate and miners should prefer it over lower sequence numbers. This was never really implemented, and the half-implemented code seemed to be making an assumption that miners would honestly prefer the higher sequence numbers, even if the lower ones were much much more profitable. That turns out to be a dangerous assumption, and so most technical people have assumed that kind of sequence number mediated replacement was useless because there was no way to enforce "honest" behavior, as even a few rational (profit maximizing) miners would break that completely. The change described by this BIP provides the missing piece that makes sequence numbers do something significant with respect to enforcing transaction replacement without assuming anything other than profit-maximizing behavior on the part of miners.
21 | +Bitcoin transactions have a sequence number field for each input. The original idea appears to have been that a transaction in the mempool would be replaced by using the same input with a higher sequence value. Although this was not properly implemented, it assumes miners would prefer higher sequence numbers even if the lower ones were more profitable to mine. However, a miner acting on profit motives alone would break that assumption completely. The change described by this BIP repurposes the sequence number for new use cases without breaking existing functionality. It also leaves room for future expansion and other use cases.
22 | +
23 | +The transaction nLockTime is used to prevent the mining of a transaction until a certain date. (in block height or time)

instagibbs commented at 6:22 PM on November 20, 2015:

drop the parenthesis, just add "in block height or time" to the sentence

btcdrak force-pushed on Nov 20, 2015

btcdrak force-pushed on Nov 21, 2015

btcdrak commented at 6:47 AM on November 21, 2015: contributor

Added an encoding diagram for clarity.

CodeShark commented at 12:05 PM on November 21, 2015: contributor

@btcdrak Beautiful! So much more clear than before. ACK

btcdrak force-pushed on Nov 21, 2015

btcdrak cross-referenced this on Nov 22, 2015 from issue BIP-68: Mempool-only sequence number constraint verification by maaku

ajtowns commented at 6:50 AM on November 23, 2015: contributor

The (1<<22) rationale is still a bit confusing to me... Maybe something like:

"The flag bit (1<<22) was chosen as the highest bit in a positive 3-byte signed integer. This allows its value to be constructed in bitcoin scripts for use with OP_CHECKSEQUENCEVERIFY (BIP 112), either by setting it directly in a straighforward manner with a 3-byte PUSHDATA, or calculating it using signed arithmetic operators."

would be better?

Otherwise, utACK fwiw.

Clarify (1 << 22) logic f26cc0c4d3

Improve title, add encoding diagram and small fixup 83fc19d97a

Rename flags in code example to match implementation 8ad8cad875

btcdrak force-pushed on Nov 23, 2015

btcdrak commented at 9:56 AM on November 23, 2015: contributor

@ajtowns I simplified the sentence. Thanks.

NicolasDorier commented at 10:01 AM on November 23, 2015: contributor

Sounds good to me, ACK

instagibbs commented at 4:07 PM on November 24, 2015: member

Carefully went through the latest draft of the BIP, and checked the related sections in the Core pull, especially LockTime(). Description and code lines up.

rustyrussell commented at 3:16 AM on November 25, 2015: contributor

Ack. This is a nice, clear change, and meets my understanding of consensus last it was discussed.

btcdrak force-pushed on Nov 26, 2015

luke-jr commented at 7:11 PM on November 28, 2015: member

This needs an ACK from @maaku

luke-jr added the label Proposed BIP modification on Nov 28, 2015

btcdrak commented at 7:17 PM on November 28, 2015: contributor

@luke-jr As you are aware @maaku stepped down from championing the proposals and handed them over to me. Its unfair to expect him to ACK this, although it would be nice if he did.

bitcoin/bitcoin#6312 is the implementation. The current master branch of BIP68 now does not match the reference implementation which the BIP points to. This is causing serious confusion. A total of three people in the last week have come asking me questions about bitcoin/bitcoin#6312 because they read the BIP and it doesn't seem to match the implementation at bitcoin/bitcoin#6312.

I might add that the implementation was updated by @maaku before he left and handed this over, however I forgot to update the text.

These modifications have been checked by @ajtowns @rustyrussell @instagibbs and @CodeShark, surely those are enough ACKs :-P

Even if I had made some error or omission after the review of the people above, it's still better than having a BIP text which is completely different to the reference implementation.

luke-jr commented at 7:23 PM on November 28, 2015: member

@btcdrak This PR includes the change of champion, so the current champion needs to ACK it. (I also have only heard of the handover from third-parties, so I don't even have personal first-hand knowledge of this.)

btcdrak commented at 7:25 PM on November 28, 2015: contributor

@luke-jr It is unfortunate that the conversations were in #bitcoin-core-dev a few days before we got channel logging setup iirc.

maaku commented at 9:53 PM on November 28, 2015: contributor

I unconditionally ACK the new list of champions.

Separately, having read the new text I suggest that acronyms be expanded. There's a couple of points where acronyms like RTL and MTP are used, even without expansion on first use. Standards should be clear and unambiguous, even if that means a little verbosity.

Small fixup a8e0d0f4c7

btcdrak force-pushed on Nov 28, 2015

luke-jr referenced this in commit 0cd94c9f72 on Nov 28, 2015

luke-jr merged this on Nov 28, 2015

luke-jr closed this on Nov 28, 2015