WebOfTrustInfo / btcr-did-tools-js

9 stars 10 forks source link

Playground showing 83 for two different DIDs — could be just coincidence #3

Open ChristopherA opened 6 years ago

ChristopherA commented 6 years ago

I have generated two DIDs using the BTCR creation tool:

TXID: 80871cf043c1d96f3d716f5bc02daa15a5e534b2a00e81a530fb40aa07ceceb6 TXID: 2f1838f481be7b4f4d37542a751aa3a27be7114f798feb24ff0fc764730973d0

The playground tool reports the first is did:btcr:x705-jzv2-qqaz-7vuz with blockheight 1353983 and index 83.

The playground tool reports for the second did:btcrxz35-jzv2-qqs2-9wjt with blockheight 1354001 and ALSO the same 83.

It could be a coincidence, but worrisome. Could be just the index isn't being output correctly, but it could be that the txref code has a bug. Going to do one more to see.

peacekeeper commented 6 years ago

With txref-conversion-java I get the same results:

80871cf043c1d96f3d716f5bc02daa15a5e534b2a00e81a530fb40aa07ceceb6
txtest1-x705-jzv2-qqaz-7vuz
ChainAndBlockLocation [chain=TESTNET, blockHeight=1353983, blockIndex=83]

2f1838f481be7b4f4d37542a751aa3a27be7114f798feb24ff0fc764730973d0
txtest1-xz35-jzv2-qqs2-9wjt
ChainAndBlockLocation [chain=TESTNET, blockHeight=1354001, blockIndex=83]

So either it's a coincidence or the Java implementation has the same bug.

kulpreet commented 6 years ago

I must be missing some details here

  1. Are we not switching to using : as the separator after the HRP?
  2. Are we not including utxo index / vout while encoding the txref part? The above txrefs don't seem to be not encoding the utxo index.
  3. Is there a current understanding on where to put the separators in the bech32 encoded txref for the nonStandard (testnet) extended (with utxo index) txrefs?

If I encode the above transactions I get the following:

[chain=TESTNET, blockHeight=1353983, blockIndex=83] txtest1:x705-jzv2-qqqq-qkx3-chl

[chain=TESTNET, blockHeight=1354001, blockIndex=83] txtest1:xz35-jzv2-qqqq-qg2u-sft

I have made the following assumptions for encoding the extended nonstandard transactions:

a. Put separators after maximum 4 characters, unless it is the last group in which case it can be 5. b. We want to encode the utxo index, even if it is 0

Please let me know if these are completely incorrect assumptions, and I will streamline my code to current approach being used for the current work.

FWIW my current implementation to encode the utxo index as part of the extended txref is in the extended branch https://github.com/kulpreet/txref/tree/extended

kulpreet commented 6 years ago

Small comment, I checked a block explorer for the two transactions and the tx index for both was 83.

ChristopherA commented 6 years ago

As I recall we leave off the txref prefix, just use the magic bytes. Yes, it should have output hyphens but accept both. I believe for compatibility with old txref, if output index is 0, you don’t encode it. Kim? Ryan?

— Christopher Allen [via iPhone]

On Tue, Jul 17, 2018 at 4:42 PM Kulpreet Singh notifications@github.com wrote:

I must be missing some details here

  1. Are we not switching to using : as the separator after the HRP?
  2. Are we not including utxo index / vout while encoding the txref part? The above txrefs don't seem to be not encoding the utxo index.
  3. Is there a current understanding on where to put the separators in the bech32 encoded txref for the nonStandard (testnet) extended (with utxo index) txrefs?

If I encode the above transactions I get the following:

[chain=TESTNET, blockHeight=1353983, blockIndex=83] txtest1:x705-jzv2-qqqq-qkx3-chl

[chain=TESTNET, blockHeight=1354001, blockIndex=83] txtest1:xz35-jzv2-qqqq-qg2u-sft

I have made the following assumptions for encoding the extended nonstandard transactions:

a. Put separators after maximum 4 characters, unless it is the last group in which case it can be 5. b. We want to encode the utxo index, even if it is 0

Please let me know if these are completely incorrect assumptions, and I will streamline my code to current approach being used for the current work.

FWIW my current implementation to encode the utxo index as part of the extended txref is in the extended branch https://github.com/kulpreet/txref/tree/extended

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/WebOfTrustInfo/btcr-did-tools-js/issues/3#issuecomment-405762240, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEN75I01j4UTW5ms0IOR57HXTFwViaEks5uHnZMgaJpZM4VR5Mx .

kulpreet commented 6 years ago

My mistake to leave the txref part in there, should have been taken out.

I don't see the BTCR playground accepting utxo index as an input parameter at the moment - https://weboftrustinfo.github.io/btcr-tx-playground.github.io/ Am I looking at the wrong version?

Also, looking at the latest merge into https://github.com/WebOfTrustInfo/txref-conversion-js I don't see the output index being ignored if it is 0.

Finally, I looked through the transpiled code for txref-conversion-js in the playground and it seems like it is not using the latest version of txref-conversion-js, i.e. it is not using the version that has support for the extended txref.

This is more me catching up with what the js playground does or doesn't do at the moment.