Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BTCR uses tx references that deviate from Bech32 Encoded Transaction Position References #1

Closed
kimdhamilton opened this issue Apr 4, 2018 · 13 comments

Comments

@kimdhamilton
Copy link
Contributor

kimdhamilton commented Apr 4, 2018

Intro

Originally, BTCR used Bech32 Encoded Transaction Position References for transaction references, but over time they deviated. We should decide (1) if we really want to do this, and if so, (2) ensure we've properly documented our deviation.

Terminology

For the moment I'll use the following placeholder terminology to refer to our modified form:

  • full name: Extended transaction reference
  • abbreviation: txref-ext
    Note that if we do choose to deviate, we should (3) decide/document the terminology

Problem

BTCR truncates Bech32 Encoded Transaction Position References (txref) by removing the network prefix. So for example, a txref of txtest1-xkyt-fzgq-qq87-xnhn converts to a txref-ext of xkyt-fzgq-qq87-xnhn. The conversion library still uses txrefs, so that we can match the testcases referred to in the reference implementation. But the BTCR libraries handle this conversion by removing/adding back the txref prefix. See the btcr-did-tools library, see util.ensureTxref.

The primary reason was to shorten the DIDs, but we discussed at RWoT 5 whether this deviation was worth it.

The following additional reasons were mentioned:

  • In the future we may also want to encode a specific tx output
  • The txref BIP won't be adopted (how do we know this?)
  • Ability to refer to alt chains
  • Ability to refer to lightning channel ids

Opening an issue to confirm this decision and follow up on items (1)-(3)

Reference:

@danpape
Copy link
Contributor

danpape commented Apr 13, 2018

Hi!

As for (1), yes, I believe we do want to do this. For (2), I'm not sure how to properly document... should we fork/edit the original BIP, or copy/augment and make a new BIP?

Another item to discuss, which you mentioned above as an "additional" reason, is adding the tx output index to the txref-ext. I've been extending the C++ library Ryan and I are developing, and I have added extra bits to the original txref definition. Here are a few examples:

txref (orig) ... implicit utxoIndex of 0
(block=0, txPos=0, uxtoIndex=0) => rqqq-qqqq-qmhu-qk
(block=466793, txPos=2205, uxtoIndex=0) => rjk0-u5ng-4jsf-mc
(block=466793, txPos=2205, uxtoIndex=10) => not possible

txref-ext (with explicit utxoIndex)
(block=0, txPos=0, uxtoIndex=0) => rqqq-qqqq-qqqu-au7hl
(block=466793, txPos=2205, uxtoIndex=0) => rjk0-u5ng-qqq8-lsnk3
(block=466793, txPos=2205, uxtoIndex=10) => rjk0-u5ng-2qqn-tcg6h

Note that I've deviated from adding a final "-" before the very last character in the txref-ext, since I thought that looked weird. We can discuss that.

@ChristopherA
Copy link
Contributor

I suggest we in parallel try to get them to modify/update their BIP — I think the generic construction might be useful to Lightning.

Also, we need to create registry in CCG of Magic Codes for chains, as BIP dropped Litecoin. There can only be a limited number of codes, so we should reserve the least useful one for extension.

@danpape
Copy link
Contributor

danpape commented Aug 21, 2018

Pinging this issue to bring it back up... Tagging @rxgrant as well.

I haven't been keeping up very much with the original discussion for the txref BIP... does that seem like something that will happen someday, or has it been overtaken by something else? Ryan and I have been using the longer txref (with embedded txo index) for some time now. We haven't gotten to submitting a pull request for our diff from the BIP-xxxx document, but I hope we will soon.

Thoughts?

@danpape
Copy link
Contributor

danpape commented Dec 14, 2018

As I'm editing various RWOT documents, I thought I would comment in here. Since the last comment in August, BIP-0136 has been updated, though still not merged in to the master set of BIPs. I have updated our C++ reference library and also Kim's javascript reference library. I would not close this issue yet, as BIP-0136 has not yet been accepted. I'll post a followup comment when I have any updates.

@kimdhamilton
Copy link
Contributor Author

Notice that I updated the btcr playground to be consistent with txref-conversion, but I didn’t strip off the chain-specific prefix. I just realized that when I saw Dan’s pr, which indicates we are still stripping that prefix off in the resulting did, deviating from the bip.

I’m feeling like our deviation isn’t worth the effort. The only reason was size reduction, right?

@ChristopherA
Copy link
Contributor

ChristopherA commented Dec 14, 2018 via email

@kimdhamilton
Copy link
Contributor Author

kimdhamilton commented Dec 14, 2018

I think there’s a cost for deviating from a standard, and I’m not sure it’s worth it in this case. I.e even if extra characters are redundant, is the reduced size really that much of a gain?

@rxgrant @danpape

Curious to hear from others

@kimdhamilton
Copy link
Contributor Author

Related question: in the original comment we called out these items as reasons for deviating:

-In the future we may also want to encode a specific tx output
-Ability to refer to alt chains
-Ability to refer to lightning channel ids

If the BIP still doesn’t address these, and we will likely deviate anyway, I will relax my opposition

@ChristopherA
Copy link
Contributor

  • I think we should recommend to them to optionally encode a specific output — this gives them the ability to be also refer to lightning nodes, which I believe to be desirable. We should probably submit a pull request to their BIP just for that feature. If accepted, we should point the lightning folks to the revised BIP.

  • My understanding that to get txref accepted as a BIP, they had to remove references to other chains. I propose that we keep a list of magic codes as another registry, or use propose a SLIP.

  • Nothing else in bitcoin ecosystem adds so many characters just for testnet. All others have a single character. I'm prefer just x prefix.

@danpape
Copy link
Contributor

danpape commented Dec 18, 2018

In the latest version of BIP-136, which veleslavs has requested to be merged (https://github.com/veleslavs/bips/blob/txrev_v2/bip-0136.mediawiki) we state that

Please note: other specifications, such as the Decentralized Identifiers spec, have implicitly
encoded the information contained within the HRP elsewhere. In this case they may choose to not
include the HRP as specified here.

After introducing the four magic codes, he also states his intention that

Other magic codes will be specified in SLIP-XXXX "TxRef for Non-Bitcoin Chains and Networks".

... though I don't know if he has any specific plans about this yet.

To summarize the magic codes: In the current BIP revision (and the current implementations of our C++ and Kim's javascript libraries) the "extended" txrefs (with tx output) are identified using new magic codes: main=3, main extended=4, test=6, test extended=7. This results in the first character of every txref being 'r', 'x', 'y' and '8', respectively. Therefore, txrefs that include the bech32 "HRP" look like:

  • main: tx1:rqqq-qqqq-qmhu-qhp
  • main-ext: tx1:yqqq-qqqq-qyrq-0ks7-gt
  • test: txtest1:xqqq-qqqq-qkla-64l
  • test-ext: txtest1:8qqq-qqqq-qyrq-pwl4-tv

Of course, once we turn them into DIDs, they start to look pretty noisy:

did:btcr:txtest1:8qqq-qqqq-qyrq-pwl4-tv

I know I have heard people say "No one should ever have to see a DID", but I like how they look without the HRP:

did:btcr:8qqq-qqqq-qyrq-pwl4-tv

@kimdhamilton
Copy link
Contributor Author

Thanks @danpape, this is helpful. It does simplify the DID strings, and I like that BIP-136 already mentions potential deviation in the HRP.

These reasons make sense to me, and I support this approach. I'll update the playground to reflect this.

@kimdhamilton
Copy link
Contributor Author

Updated playground, but I'll keep this issue open until we document these decisions in the relevant places

@kimdhamilton
Copy link
Contributor Author

the ability to drop the HRP is documented in BIP 136, and we address this in the method spec. Ok to close

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

No branches or pull requests

3 participants