Page MenuHomePhabricator

[alias] spec overview before alias-server implementation
ClosedPublic

Authored by bytesofman on Jun 5 2023, 20:44.

Details

Reviewers
Fabien
Group Reviewers
Restricted Project
Commits
rABCb0b8f54d94f6: [alias] spec overview before alias-server implementation
Summary

T3060

Following review of D13916, practical to keep any spec modifications to their own diff, and good idea to do this before they are actually implemented.

Test Plan

Review changes for consistency with desired approach.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
alias-server-latest-spec-latest-methods
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 24004
Build 47618: Build Diff
Build 47617: arc lint + arc unit

Event Timeline

Fabien requested changes to this revision.Jun 6 2023, 07:14
Fabien added a subscriber: Fabien.
Fabien added inline comments.
doc/standards/ecash-alias.md
64 ↗(On Diff #40602)

Why do want to open to more push options ?

84 ↗(On Diff #40602)

Does your address encoding/decoding library support hash lengths different from 20 bytes ? Because right now they are invalid on the network. It's good to link to the spec, but for now we should restrict to 20 bytes addresses only.

This revision now requires changes to proceed.Jun 6 2023, 07:14
doc/standards/ecash-alias.md
42 ↗(On Diff #40602)

prettier is adding this new line on my machine. separate issue that I still haven't clarified which repos prettier is hitting with arc lint.

64 ↗(On Diff #40602)

imo this should always just be '04', since all of the other ways of pushing for this are inefficient (take up more space than necessary to do the same thing).

But this is also true for all the other pushes of the spec (no pushes can be long enough to require 4c, let alone 4d or 4e).

However, now that we have a library which arbitrarily returns any valid OP_RETURN push, enforcing this would require additional logic to check not only the next valid push but also something like isMinimalPush: true. Not sure if this complication is worth it, given that anyone making such weird pushes is probably rare enough...and at any rate they must pay the appropriate fee to get the tx on the network.

If it's a valid tx, should probably take it. It would be nice if there were only one valid way to push a 4 byte protocol identifier at the protocol level but perhaps impractical to adjust there for other reasons.

68 ↗(On Diff #40602)

Note: the logic of parseAliasTx will need to be further adjusted to confirm the version 00 is a one-byte push of OP_0. If we just parse the whole OP_RETURN with consumeNextPush, then 4e0100000000 will also be valid.

...so perhaps there is a case to be made for isMinimalPush to be supported in the op_return library

84 ↗(On Diff #40602)

interesting, TIL.

Yes, the cashaddress library supports the following hash sizes:

// Throw validation error if hash is of invalid size
// Per spec, valid hash sizes in bytes
const VALID_SIZES = [20, 24, 28, 32, 40, 48, 56, 64];

Require minimal pushes, require valid addresses

doc/standards/ecash-alias.md
64 ↗(On Diff #40602)

The reason for enforcing minimal push is that you can manage the protocol extension with no risk of overflowing the data carrier max size.
I think you made a very good point that enforcing minimal push is a good move :)

84 ↗(On Diff #40602)

That's great. Sill we should restrict to 20 bytes for now as it will not be interoperable with other tools of the ecosystem

bytesofman added inline comments.
doc/standards/ecash-alias.md
84 ↗(On Diff #40602)

ok -- reverted / updated

bytesofman marked an inline comment as done.

Simplify indexing by requiring single OP_RETURN output at first position to be valid

Actually if we want to do minimum pushes, better to support eMPP spec https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/chronik/bitcoinsuite-slp/src/empp/mod.rs

In this way:

  • We can lose the "04" for protocol identifier push
  • We can lose the "15" for address type + hash push
  • We can lose the 0x01-0x15 for alias length push (by putting the alias at the end, we just say "the rest of this push is the alias"

We need to add OP_RESERVED but still save a couple bytes. And of course, would also enable other app pushes in the same tx.

That said, we should prob support both (legacy with min push and empp)

Actually if we want to do minimum pushes, better to support eMPP spec https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/chronik/bitcoinsuite-slp/src/empp/mod.rs

In this way:

  • We can lose the "04" for protocol identifier push
  • We can lose the "15" for address type + hash push
  • We can lose the 0x01-0x15 for alias length push (by putting the alias at the end, we just say "the rest of this push is the alias"

We need to add OP_RESERVED but still save a couple bytes. And of course, would also enable other app pushes in the same tx.

That said, we should prob support both (legacy with min push and empp)

Update

While I think this would be cool,

  1. Would be easy to add this support later without impacting the baseline spec
  2. There's no obvious need to have this support before launch

So, will launch without eMPP support

Update spec for txs with multiple OP_RETURN outputs

Fabien added inline comments.
doc/standards/ecash-alias.md
54

nit: will be => is

88

You could sneak in this diff another change, this should be a finalized transaction (not a 1 block confirmed tx)

This revision is now accepted and ready to land.Jun 13 2023, 06:27