Page MenuHomePhabricator

[Chronik] Add `parse_tx_lokad_ids` to parse LOKAD IDs
ClosedPublic

Authored by tobias_ruck on Sat, Apr 6, 04:34.

Details

Summary

Allows detecting and grouping txs that have some protocol that follows the extended LOKAD ID spec.

We allow the following patterns for a LOKAD ID:

  • OP_RETURN <LOKAD ID> ... (first output), where the LOKAD ID is a 4-byte pushdata op. This is the only allowed variant in the original spec, and still kind-of the standard.
  • OP_RETURN OP_RESERVED "<LOKAD ID>..." "<LOKAD_ID>..." ... (first output), where the OP_RETURN is encoded as eMPP and every pushop is considered prefixed by a 4-byte LOKAD ID. This is new after the introduction of eMPP.
  • <LOKAD ID> ... (every input), where any input starting with a 4-byte pushop is interpreted as a LOKAD ID. This allows covenants to easily enforce a LOKAD ID by simply doing <LOKAD ID> OP_EQUAL at the end of all the ops.
Test Plan

cargo test -p bitcoinsuite-slp --lib

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tobias_ruck updated this revision to Diff 46903.

rerun lint

Will need to name types for the iterator; -> impl Iterator doesn't work with Group

Failed tests logs:

====== Get blocktxs, txs, and history for SLP NFT1 token txs: "before each" hook for "Gets an SLP NFT1 child genesis tx from the mempool".Get blocktxs, txs, and history for SLP NFT1 token txs "before each" hook for "Gets an SLP NFT1 child genesis tx from the mempool" ======
Error: Timeout of 60000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/work/modules/chronik-client/test/integration/token_slp_nft1.ts)
    at listOnTimeout (node:internal/timers:573:17)
    at processTimers (node:internal/timers:514:7)

Each failure log is accessible here:
Get blocktxs, txs, and history for SLP NFT1 token txs: "before each" hook for "Gets an SLP NFT1 child genesis tx from the mempool".Get blocktxs, txs, and history for SLP NFT1 token txs "before each" hook for "Gets an SLP NFT1 child genesis tx from the mempool"

bytesofman requested changes to this revision.Mon, Apr 8, 13:55
bytesofman added a subscriber: bytesofman.

imo this is a great add. Would be very useful for app devs to look up txs by lokad id. for example, a chat app would be almost trivial if you could look up txs by lokad id and address.

only taking the index 0 op_return is imo an acceptable spec but would like to see it documented / referenced somewhere, say here: https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/doc/standards/op_return-prefix-guideline.md

Can be added after this diff pending comment responses.

Pros: we remove the hard-to-handle-but-basically-never-happens edge case of a tx with multiple op_returns
Con: I could see a new app dev putting op_return output at an index other than 0 without really thinking about it.

Would be nice to just take the op_return output with the lowest index. imo anyone custom rolling a multi-op-return tx doesn't really need hand holding index support. but it would be nice to cover the case of app dev not familiar with a poorly documented spec.

Is this practical or would the impact of this not justify the additional indexing complexity?

ok to green this as-is but would like some more info about the spec / reason for only indexing 0-index OP_RETURNs

chronik/bitcoinsuite-slp/src/lokad_id.rs
101–102 ↗(On Diff #46939)

is there a good ref for this original spec?

this means we are only looking for a lokadId in the index-0 output and will not index if there is, say, an op_return at index 1?

imo this is fine and makes indexing much easier. there is not really an app dev need (that I am aware of) for OP_RETURN outputs to be at some other index.

would be nice though to have this documented somewhere that app devs could find it.

This revision now requires changes to proceed.Mon, Apr 8, 13:55

The original spec is here: https://github.com/bitcoincashorg/bitcoincash.org/blob/master/spec/op_return-prefix-guideline.md, this is already referenced in our version of that file.

only taking the index 0 op_return is imo an acceptable spec but would like to see it documented / referenced somewhere, say here: https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/doc/standards/op_return-prefix-guideline.md

Yes; there's three options on this front that I see:

  1. Add the new spec used here to the guideline (referencing the bitcoincash.org repo)
  2. Like 1. but also integrate the original op_return-prefix-guideline.md from the bitcoincash.org repo into our guideline
  3. Move our current guideline file to a different one and copy the original guideline from the bitcoincash.org repo into our repo unchanged, and reference that file internally

Pros: we remove the hard-to-handle-but-basically-never-happens edge case of a tx with multiple op_returns
Con: I could see a new app dev putting op_return output at an index other than 0 without really thinking about it.

IMO the con is pretty small: the dev will figure it out relatively quickly anyway, it's quite natural to place it there, and we can make it abundantly clear that it has to be at output index 0.

Would be nice to just take the op_return output with the lowest index. imo anyone custom rolling a multi-op-return tx doesn't really need hand holding index support. but it would be nice to cover the case of app dev not familiar with a poorly documented spec.

I think the solution is to have a better documented spec then...

Is this practical or would the impact of this not justify the additional indexing complexity?

It's totally practical, but I think overall it's better to always have the OP_RETURN at the first output index, it'll have a lot of positive downstream effects, most importantly that it'll be easier for new devs to not get lost reading existing txs (or rather code dealing with it), and for medium-experienced devs to have a trained pattern-maching for the OP_RETURN to be at position 0 when indexing.

Most importantly, I think, SLP already enforces the OP_RETURN to be at 0 (for very good reasons), likewise eMPP, and it could confuse users if they suddenly encounter a protocol that doesn't work the same way.

I see no advantage to having the OP_RETURN not be at output index 0, except for when it slightly optimizes some Script covenants, but that doesn't seem that necessary all things considered.

This revision is now accepted and ready to land.Tue, Apr 9, 23:49