-
Notifications
You must be signed in to change notification settings - Fork 296
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(chain): calculate DescriptorId as the sha256 hash of spk at index 0 #1486
refactor(chain): calculate DescriptorId as the sha256 hash of spk at index 0 #1486
Conversation
3a9c3ad
to
95f76e0
Compare
I don't feel doing a blind wildcard replacement is worth the trouble here and actually interferes with the design @evanlinjin is implementing I think. It should just be the SPK at index 0 while leaving the descriptor as is. |
I'm not a fan of doing the wildcard-replacement as it creates a behavior that I find hard to reason with: We cannot have 2 non-wildcard descriptors that are part of the same keychain (even though their derived spks do not conflict). I think it's better to disallow non-wildcard descriptors in I'm going to provide a few suggestions that I think is enough for us to move into
I'm already working on renaming the |
Thanks for the feedback, I'll update this PR as suggested. @evanlinjin I'd still like to have some way to sweep single SPK descriptors into a |
95f76e0
to
79f02d3
Compare
79f02d3
to
d0a9982
Compare
There are still a few tests failing that I could use some help fixing. |
4377f16
to
74e498b
Compare
I had to ignore 3 PSBT tests and 1 Wallet test, added TODOs to fix them during 1.0.0-beta phase. |
We should also open an issue no (if this is merged)? TODO comments get lost and forgotten quite quickly |
2425c01
to
518f3e9
Compare
I simplified this PR back to allowing wildcard and non-wildcard descriptors in a I update docs to explain that it's the developer's responsibility to ensure that wildcard and non-wildcard descriptors don't derive the same spk if used in the same |
@evanlinjin if you're OK with this solution I'd like to move it to the beta milestone since it won't change the wallet API. |
@notmandatory this is a breaking change because |
@evanlinjin ah ok I wasn't thinking about the changeset, I'll keep this is the alpha milestone. |
@notmandatory I've been thinking about this. The problem with your solution is that any descriptor/key can be turned into a If we want to support single spk descriptors, I think we should do it properly. The way to do this is to have two versions of I would first focus on a keychain wallet first and get it correct. Then we can build a single-spk wallet later (it shouldn't be too hard?). |
@evanlinjin I'd rather not do any more redesign on |
The problem is the Edit: Okay I guess for actual keychains it'll be the same. We won't have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some docs to update, but looks good to me.
518f3e9
to
cb5db7f
Compare
Maybe this is just semantics, but technically even a non-wildcard descriptor could be called a keychain since as currently coded it's pks still derive a key at each index, they're just all the same. 🙃 |
Sure, but as long as there is a pathway to achieve correctness (perfect uniqueness) of |
I would be happy to ACK and merge this soon so I can base my changes on this. I am moving |
cb5db7f
to
bdc9659
Compare
I also added a little test to clarify what happens if overlapping keychains are used in a |
bdc9659
to
32264ac
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR was better when it didn't rename DescriptorId
to KeychainId
. There is not concept of a Keychain
in this PR and only descriptors have a KeychainId
apparently. In my mind this should have just been about calculating DescriptorId
differently. Anyway @evanlinjin explicitly requested it....
I think it's better to disallow non-wildcard descriptors in KeychainTxOutIndex. This way, DesciptorId is always guaranteed to be unique.
Unfortunately this is not the case. wsh(multi(2,xpubA/42, xpubB/*)
and wsh(multi(2, xpubA/*, xpubB/42)
have the same spk at index 42
but different at every other index. It is actually impossible to ensure that a malicious party cannot generate two completely different descriptors that happen to have the same spks at two different derivation indices. This is because wpkh
, pkh
and sh
descriptors all use 20 byte hashes which are not sufficiently collision resistant.
Once again in order to have this "zero collisions" property you must enforce it at the application level. If this needs to be enforced against a malicious adversary who can insert whatever descriptor they want then you must disallow all the 20-byte hash spk descriptor types and make sure that all keys are xpubs with wildcards...I think! We can document this but there's nothing else we can do. The design here is still right. If we can make what happens when there are collisions less surprising that's good but otherwise we should document and ignore this problem.
crates/chain/src/descriptor_ext.rs
Outdated
fn keychain_id(&self) -> KeychainId { | ||
let spk = self.at_derivation_index(0).unwrap().script_pubkey(); | ||
let spk_bytes = <Vec<u8>>::from(spk.as_bytes()); | ||
KeychainId(Hash::hash(&spk_bytes)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛏️ can you put sha256::Hash::hash or something. It's not clear which hash algorithm you're using.
⛏️ you shouldn't need to allocate a Vec
here. Why can't you just put the as_bytes
into the hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed.
32264ac
to
599d6a8
Compare
I rolled back the |
Is my current warning for |
Thanks. The PR is now much easier to review without the name change. Here's my attempt at documenting this:
|
Also update docs to explain how KeychainTxOutIndex handles descriptors that generate the same spks.
599d6a8
to
8f5b172
Compare
@LLFourn I updated the docs with your much more nuanced explanation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK 8f5b172
ACK 8f5b172 |
Description
Rename
DescriptorId
toKeychainId
anddescriptor_id()
tokeychain_id()
.Calculate keychain ids as the hash of the spk derived from its descriptor as index 0.
Added docs to
Wallet
andKeychainTxOutIndex::insert_descriptor()
explaining that it's the users responsibility not to use wildcard and non-wildcard descriptors that can derive the same script pubkey. I also recommended forWallet
that legacy non-wildcard descriptors be added to a temporaryWallet
and swept into a modern wildcard descriptor.Notes to the reviewers
fixes #1483
Changelog notice
changed
Checklists
All Submissions:
cargo fmt
andcargo clippy
before committingBugfixes: