Use even/odd TLV numbers everywhere #547
Conversation
| WitnessSplitCommitment WitnessTlvType = 2 | ||
| WitnessPrevID WitnessTlvType = 1 | ||
| WitnessTxWitness WitnessTlvType = 3 | ||
| WitnessSplitCommitment WitnessTlvType = 5 |
There was a problem hiding this comment.
I think this is the only one amongst them that should be odd?
There was a problem hiding this comment.
Same here, both the PrevID and TxWitness fields have an explicit check in EncodeRecords:
Line 340 in 5373ccc
I guess if we declare PrevID as required, it would no longer need to be a pointer? Not sure how to decide what to take as the source of truth for the decision: our current implementation or the fact that it should in practice never be empty...
There was a problem hiding this comment.
Indeed, its the ZeroPrevID in both forms of the genesis witness, and the VM already fails if its nil. In favor of promoting to not-pointer.
TxWitness is fine as-is though IIUC should only be empty for genesis assets.
| AnchorTxType tlv.Type = 6 | ||
| TxMerkleProofType tlv.Type = 8 | ||
| AssetLeafType tlv.Type = 10 | ||
| InclusionProofType tlv.Type = 12 |
There was a problem hiding this comment.
One thing we even odd that we could do here is actually assign in pairs. We do this for feature bits in LN so we can flip them to required if needed.
In this case, we would have ExclusionProofsType actually be 15.
Not sure if this is useful though?
There was a problem hiding this comment.
Ah, I see. Yes, that could be nice so we could turn things on/off when it comes to being required. Happy to do the change if you think we should. But then I'd say we should update the address records as well.
There was a problem hiding this comment.
Don't feel super strongly about this, as in the future if we wish we can use a new version to redo all the types. So far we don't have a concrete need for this as is though.
There was a problem hiding this comment.
Yeah, me neither... So I guess let's keep it the way they are now?
guggero
force-pushed
the
tlv-re-numbering
branch
from
a8df488
to
5fa2501
Compare
guggero
force-pushed
the
tlv-re-numbering
branch
from
5fa2501
to
1d164a8
Compare
|
Change in |
guggero
force-pushed
the
tlv-re-numbering
branch
from
1d164a8
to
8ef9f9d
Compare
Now that we have proper test vectors that we can update automatically, we no longer need the hard coded single test case in the asset.hex file.
Now that we have proper test vectors that can be updated automatically, we no longer need the hard coded single test cases in the psbt.b64 and psbt.hex files.
guggero
force-pushed
the
tlv-re-numbering
branch
from
8ef9f9d
to
176aabd
Compare
| AnchorTxType tlv.Type = 6 | ||
| TxMerkleProofType tlv.Type = 8 | ||
| AssetLeafType tlv.Type = 10 | ||
| InclusionProofType tlv.Type = 12 |
There was a problem hiding this comment.
Don't feel super strongly about this, as in the future if we wish we can use a new version to redo all the types. So far we don't have a concrete need for this as is though.
|
Lost track of if was this PR or another, but: I think we can pull out the changes to the restriction/validation of the meta field into another PR. |
I think what you mean was in #525, which was merged by now. |
Definitely and finally closes #347.
Corresponding BIP PR is here: Roasbeef/bips#43