370: Clarifications of locktimes and tx modification flags #1328

achow101 commented at 11:58 PM on June 1, 2022: member

No description provided.

in bip-0370.mediawiki:221 in e9640cf1e4 outdated

 217 | +If one or more inputs have a PSBT_IN_REQUIRED_TIME_LOCKTIME or PSBT_IN_REQUIRED_HEIGHT_LOCKTIME, then the field chosen is the one which is supported by all of the inputs.
 218 |  This can be determined by looking at all of the inputs which specify a locktime in either of those fields, and choosing the field which is present in all of those inputs.
 219 |  Inputs not specifying a lock time field can take both types of lock times, as can those that specify both.
 220 |  The lock time chosen is then the maximum value of the chosen type of lock time.
 221 |  
 222 | +If a PSBT has both types of locktimes possible, then locktime determined by looking at the PSBT_IN_REQUIRED_HEIGHT_LOCKTIME fields of the inputs must be chosen.<ref>'''Why choose the height based locktime?'''

jgriffiths commented at 12:10 AM on June 2, 2022:

Suggest: "If a PSBT has both types of locktime possible because one or more inputs specify both PSBT_IN_REQUIRED_TIME_LOCKTIME and PSBT_IN_REQUIRED_HEIGHT_LOCKTIME, ..."

The case where both types are implicitly possible by virtue of not being provided should fall back to PSBT_GLOBAL_FALLBACK_LOCKTIME or 0.

achow101 commented at 1:16 AM on June 2, 2022:

Done

jgriffiths commented at 12:17 AM on June 2, 2022: none

Should we also prevent 0 as an allowable value in the description for PSBT_IN_REQUIRED_HEIGHT_LOCKTIME?

370: Describe locktime type tiebreaker 1c8afe6ea4

370: height lock must be greater than 0 aa807d5a13

achow101 force-pushed on Jun 2, 2022

achow101 commented at 1:16 AM on June 2, 2022: member

Should we also prevent 0 as an allowable value in the description for PSBT_IN_REQUIRED_HEIGHT_LOCKTIME?

Done.

jgriffiths commented at 1:42 AM on June 2, 2022: none

ack aa807d5a139cb36cf258553da4510142edf53edb (discussion, review)

in bip-0370.mediawiki:106 in aa5c2f04db outdated

 102 | @@ -103,7 +103,7 @@ The new global types for PSBT Version 2 are as follows:
 103 |  | None
 104 |  | No key data
 105 |  | <tt><8-bit uint></tt>
 106 | -| An 8 bit little endian unsigned integer as a bitfield for various transaction modification flags. Bit 0 is the Inputs Modifiable Flag and indicates whether inputs can be modified. Bit 1 is the Outputs Modifiable Flag and indicates whether outputs can be modified. Bit 2 is the Has SIGHASH_SINGLE flag and indicates whether the transaction has a SIGHASH_SINGLE signature who's input and output pairing must be preserved. Bit 2 essentially indicates that the Constructor must iterate the inputs to determine whether and how to add an input.
 107 | +| An 8 bit little endian unsigned integer as a bitfield for various transaction modification flags. Bit 0 is the Inputs Modifiable Flag and indicates whether the inputs can be added or removed. Bit 1 is the Outputs Modifiable Flag and indicates whether outputs can be added or removed. Bit 2 is the Has SIGHASH_SINGLE flag and indicates whether the transaction has a SIGHASH_SINGLE signature who's input and output pairing must be preserved. Bit 2 essentially indicates that the Constructor must iterate the inputs to determine whether and how to add an input.

jgriffiths commented at 11:40 PM on June 2, 2022:

determine whether and how to add an input

suggest determine whether and how to add or remove an input

jgriffiths commented at 11:41 PM on June 2, 2022:

An 8 bit little endian unsigned integer

Suggest An 8 bit unsigned integer, there is no such thing as a big/little endian byte.

achow101 commented at 12:00 AM on June 3, 2022:

The bits do not specify a how, so that would be incorrect to state.

Fixed the endian thing.

jgriffiths commented at 2:15 AM on June 3, 2022:

The "how" part is already in the text which says: "to determine whether and how to add an input".

I'm suggesting changing to add into to add or remove. If you think and how should be removed no objection here.

achow101 commented at 2:38 AM on June 3, 2022:

Ah, I see. I thought you were talking about a different section. Changed that.

achow101 force-pushed on Jun 2, 2022

370: clarify inputs/outputs modifiable

Clarify that these flags only mean whether inputs/outputs can be
removed, not whether fields can be added or removed for each
input/output.

5861862f59

achow101 force-pushed on Jun 3, 2022

jgriffiths commented at 1:03 AM on June 7, 2022: none

I have another suggested change.

It should be made more clear that the tx modifiable flags must be set to enable modification rather than to disable it - currently this has to be inferred from a sentence on the constructor role later in the BIP.

Bit 0 is the Inputs Modifiable Flag and indicates whether inputs can be modified. Bit 1 is the Outputs Modifiable Flag and indicates whether outputs can be modified. Bit 2 is the Has SIGHASH_SINGLE flag and indicates whether the transaction has a SIGHASH_SINGLE

should be:

Bit 0 is the Inputs Modifiable Flag, set to 1 to indicate that inputs can be added or removed. Bit 1 is the Outputs Modifiable Flag, set to 1 to indicate that outputs can be added or removed. Bit 2 is the Has SIGHASH_SINGLE flag, set to 1 to indicate that the transaction has a SIGHASH_SINGLE

Then to clarify further and correct boolean vs bits:

A Constructor may choose to declare that no further inputs and outputs can be added to the transaction by setting the booleans in PSBT_GLOBAL_TX_MODIFIABLE to False

should be:

A Constructor may choose to declare that no further inputs or outputs can be added to the transaction by setting the appropriate bits in PSBT_GLOBAL_TX_MODIFIABLE to 0

370: Clarify that modifiable flags are bits 02ab2bfd79

achow101 commented at 4:22 PM on June 13, 2022: member

@jgriffiths Done.

achow101 renamed this:
~~370: Describe locktime type tiebreaker~~
370: Clarifications of locktimes and tx modification flags
on Jun 13, 2022

jgriffiths commented at 10:11 PM on June 13, 2022: none

ack 02ab2bfd79d860208fe09e7b2afa12541aba5eb2

I've been able to implement v2 support in wally with these clarifications and testing shows no more inconsistencies so far.

luke-jr merged this on Jul 25, 2022

luke-jr closed this on Jul 25, 2022