Date: Mon, 9 Jan 2023 18:11:05 +1000
From: Anthony Towns <aj@erisian.com.au>
To: Bitcoin Protocol Discussion <bitcoin-dev@lists.linuxfoundation.org>
Message-ID: <Y7vMGVQz8TjS4Cad@erisian.com.au>
References: <56677685-619a-691f-d5bc-54b69fdb6ed2@bip324.com>
 <zxv58iXZ73hf9ge8S0QLTanW-uLzaWjNtMHuKONP9hrqS5RhwitxzfVaMH8hbi3yImgNrKme3lCuDcHYKkpxEQHyGZZHJ8xtReOcnAx3o4g=@wuille.net>
 <Y2nK99fHUKxbPHmw@erisian.com.au>
 <wDqcIVw-YGTsjdf5M2GO9NNRl_UQuBeka2CUQUyQ329u6u-o7RabW_7S4FD3EDfk02kUczb3bXf8LtHhKLtx773UhQ7djKOl-JPOIrXqBSc=@wuille.net>
 <JXfTBjsA71dHE3h9wkxnWXANrwTbMADO4s2w34gEvMbiduKu4PEt5t-KA3EAIz-Xs4urjBHZ15NDFZST2a7e0x_NqyJymUnEORuTp3SNfMs=@wuille.net>
 <Y3dBUXPhTskCx+Fu@erisian.com.au>
 <gSxFQedPc72pTioi9vuxvLKpaRBsnKFL4gkPKPn2G-EJgz_2Y1pYQ7cHD5SnunyCaLln7UQEHIxnopqP74LlnK__Mf9BURbJW8B5MYTZvCU=@wuille.net>
 <Y7dZctMlZtH6PEsz@erisian.com.au>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y7dZctMlZtH6PEsz@erisian.com.au>
Subject: Re: [bitcoin-dev] Refreshed BIP324
Precedence: list

On Fri, Jan 06, 2023 at 09:12:50AM +1000, Anthony Towns via bitcoin-dev wrote:
> On Thu, Jan 05, 2023 at 10:06:29PM +0000, Pieter Wuille via bitcoin-dev wrote:
> > Oh, yes. I meant this as an encoding scheme, not as a (replacement for) the negotiation/coordination mechanism. There could still be an initial assignment for 1-byte encodings, and/or an explicit mechanism to negotiate other assignment, and/or nothing at all for now.

> The current implementation for 324 does the aliasing
> as part of V2TransportDeserializer::GetMessage and
> V2TransportSerializer::prepareForTransport. That makes a lot of sense,
> [...]

So I think you can make this setup work with a negotiated assignment of
shortids, perhaps starting off something like:

https://github.com/ajtowns/bitcoin/commit/6b8edd754bdcb582e293e4f5d0b41297711bdbb7

That has a 242 element array per peer giving the mappings (which
is just ~250 bytes per peer) for deserialization, which seems
workable. [0]

It also has a single global map for serialization, so we'll always shorten
CFILTER to shortid 39 for every peer that supports shortids, even, eg, for
a peer who's told us they'll send CFILTER as shortid 99 and that we should
interpret shortid 39 from them as NEWFEATUREX. That has three advantages:

 * each peer can choose a mapping that minimises their own outbound
   traffic, even potentially for asymmetric connections, and don't need
   to coordinate with the other peer to decide a common optimal mapping
   that they both use across their connection

 * you don't have to have different serialization tables per-peer,
   reducing memory usage / implementation complexity

 * you can leave V2TransportSerializer as a `const` object, and not have
   to introduce additional locking logic to be able to update its
   state...

I'm not seeing a good way to introduce shortids for future one-shot
negotiation messages though (like VERSION, VERACK, SENDADDRV2,
WTXIDRELAY, SENDTXRCNCL):

 * if you explicitly announce the mapping first, you're just wasting
   bytes ("99=FOOBAR; 99 baz quux" vs just "FOOBAR baz quux")
 * if you negotiate the tables you support between VERSION/VERACK and
   then choose a mutually supported table after VERACK, that's too late
   for pre-VERACK negotation messages
 * announcing the tables you support as part of the VERSION message
   would work, but seems a bit klunky

Also, if you did want to shift to a new table, you'd probably want to
always support sending/receiving {37, 44, 46, 47, 36} messages?

I guess I still kind-of think it'd make more sense to just reserve
shortids for post-VERACK messages that are going to be sent more
than once per connection... At that point, even if you don't have any
table in common with your peer, just following VERACK with an immediate
announcement of each shortid you want to use and its meaning would still
make reasonable sense.

If we included the ability to define your own shortids concurrently
with bip324 rollout, then I think nodes could always have a static set
of shortids they use for all their peers for outbound messages, which,
as above, seems like it would make for simpler implementations.

ie, you might send:

   VERSION
   SHORTIDTBLS ["","awesomeshortids"]
   WTXIDRELAY
   SENDADDRV2
   SENDPACKAGES 1
   VERACK
   SHORTID "" [(52,"getpkgtxns"), (53, "pkgtxns"), (54, "ancpkginfo")] 

...but you'd do all that long form, and only switch to shortids for
messages after you've declared exactly what your shortids are going to
be.

(where "" is the table name for bip324's table, and "awesomeshortids"
is an updated table that includes the package relay commands already,
perhaps)

Cheers,
aj

[0] m_deserializer is used from the SocketHandler thread in
    CNode::ReceiveMsgBytes(), but the p2p protocol is managed from the
    MessageHandler thread; with multiple messages potentially deserialized
    into vRecvMsg() at once -- but that means that if the first message
    redefines shortid decoding, and the second message uses one of the
    redefined shortids, it will have already been decoded incorrectly.
    So that would need some futzing about still.