Shorten subdirectory IDs #386
Conversation
|
Akshually, I just realized the last commit message |
nothingmuch
force-pushed
the
short-ids
branch
2 times, most recently
from
b8d3332
to
a6b64ed
Compare
|
Concept ACK in response to the "Shorten subdirectory IDs to 64 pseudorandom bits" commit message. ✅ utxo set size ≈ 2^28
Concept ACK also on your note that this already improves collision resistance on the naive truncated shortIDs of not-completely-pseudorandom values. |
nothingmuch
force-pushed
the
short-ids
branch
from
a6b64ed
to
bc984d9
Compare
nothingmuch
force-pushed
the
short-ids
branch
from
bc984d9
to
fc8ff6c
Compare
nothingmuch
force-pushed
the
short-ids
branch
from
fc8ff6c
to
036223a
Compare
There was a problem hiding this comment.
This appears to do what it says it does to me. There are quite a few TODOs that are OK to do in follow ups. I find it a bit weird to change the API in the first commit and only to use it in later commits but that's a nit of nits
Hoping @spacebear21 weighs in as well
Instead of the compressed public key, subdirectory IDs are now a truncated SHA256 of the compressed public key. These are should only be treated unique identifiers, not hashes: the use of SHA256 is only an implementation detail, and should not be specified by BIP77, nor verified/enforced by clients. This is because 64 bit hashes are insufficiently binding: finding a pair of colliding keys is almost trivial and finding a 2nd preimage for a given ID is tractable. For this reason no tagging is used to derive the IDs: public keys are ephemeral and have sufficient entropy to be unguessable. Random IDs could also have been used, but hashing seems simpler and reduces the receiver's statefulness requirements. ID collisions are only a liveness concern, and do not affect safety. With BIP77, HPKE will fail due to the wrong key (and/or domain separation if future protocols also use short IDs). With BIP 78 fallback the PSBT will not contain the intended receiver's outputs. The intended receiver can still poll the same subdirectory and respond, eventually, but only one sender will succeed. 64 bits are sufficient to make the probability of experiencing a random collisions negligible. As of writing, the UTXO set has ~2^28 elements. This is a very loose upper bound for the number of concurrent (non-spam) sessions, for which the probability of a random collision with will be <1%. The actual number of sessions will of course be (orders of magnitudes) lower given they are short lived. With ~2^21 sessions (loose bound on number of transactions that can be confirmed in 24H) the probability is less than 1e-6. These figures are for the existence of a collision in the set, the probability for an individual session to experience a random collision is << 1e-10 in either case. Since no rate limiting or access control mechanism exists for the directory yet, it's notable that this change changes the nature of a hypothetical DoS attack. With long IDs the adversary could only cause operational errors in theory (e.g. by filling the directory's storage). Note that by polling a large number of IDs an adversary can succeed in randomly *intercepting* v2 clients' sessions, and POST garbage data to the session causing HPKE to fail. For v1 sessions this can leak PSBT proposals, since those are not encrypted, which can leak input ownership information to the adversary. As implemented this change is not a regression but an actually hardens against DoS/malice in practice, because although in theory subdirectory IDs contained more entropy, the underlying redis keys prior to this change only contained 41 bits of entropy (8 characters of base64 encoded data, with 0x02 or 0x03 for the first encoded byte). Both the random collision and abuse scenarios can be mitigated by restricting the number of concurrent sessions in the directory to more reasonable values (less than 2^20). This is not done in this change.
nothingmuch
force-pushed
the
short-ids
branch
from
036223a
to
a95a2ba
Compare
Squashed first two commits |
There was a problem hiding this comment.
utACK a95a2ba. Noted one more nit that can be addressed in a follow-up.
| @@ -99,6 +117,6 @@ impl DbPool { | |||
| } | |||
| } | |||
|
|
|||
| fn channel_name(pubkey_id: &str, channel_type: &str) -> String { | |||
| format!("{}:{}", pubkey_id, channel_type) | |||
| fn channel_name(pubkey_id: &ShortId, channel_type: &str) -> Vec<u8> { | |||
There was a problem hiding this comment.
This helper is no longer that helpful IMO and could be replaced by direct calls to column_key() in later commits.
There was a problem hiding this comment.
agreed... do you think the TODO comments for ShortId are missing anything or could be simplified?
WIP since this should probably be refactored a bit before merging (see TODO comments).
The message for the last commit contains a rationale for 64 bit values and hashing.