Skip to content
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

Do not include satisfaction in policy id calculation #1218

Open
Tracked by #86 ...
afilini opened this issue Nov 15, 2023 · 6 comments
Open
Tracked by #86 ...

Do not include satisfaction in policy id calculation #1218

afilini opened this issue Nov 15, 2023 · 6 comments
Labels
api A breaking API change module-wallet new feature New feature or request

Comments

@afilini
Copy link
Member

afilini commented Nov 15, 2023

Currently every node in the Policy tree has an id which is calculated as a checksum on the string of the json-serialized object:

/// Returns a unique id for the [`SatisfiableItem`]
pub fn id(&self) -> String {
calc_checksum(&serde_json::to_string(self).expect("Failed to serialize a SatisfiableItem"))
.expect("Failed to compute a SatisfiableItem id")
}

The object includes details about whether the current wallet can satisfy the policy in some ways (aka contribution), and these are also included in the checksum. The same is true for what in a psbt is currently satisfied, which is the satisfaction field of Policy.

This causes the id of the nodes to change from wallet to wallet, depending on which private keys a wallet has or depending on which psbt you are looking at. Ideally if both wallets have the same public descriptor, they should also have the same id for the nodes, which makes sharing policy_paths much easier.

I would recommend changing the way the id is calculated, if you have any ideas let me know. I could also try working on it myself.

@afilini afilini added the new feature New feature or request label Nov 15, 2023
@notmandatory notmandatory added this to BDK Nov 15, 2023
@notmandatory notmandatory moved this to Todo in BDK Nov 15, 2023
@notmandatory notmandatory added this to the 1.0.0-alpha.4 milestone Nov 15, 2023
@notmandatory
Copy link
Member

@matthiasdebernardini I believe your project uses this feature pretty heavily, any thoughts?

@LLFourn
Copy link
Contributor

LLFourn commented Nov 16, 2023

I was going to suggest simply removing the policy module in favor of pointing people to the planning module in miniscript. Thoughts?

@afilini
Copy link
Member Author

afilini commented Nov 16, 2023

Yeah that would be better, but as far as I know it still hasn't been released, right?

@LLFourn
Copy link
Contributor

LLFourn commented Nov 21, 2023

It's still not released yeah :\

@matthiasdebernardini
Copy link

@LLFourn I was gonna ask, where can I see this module? I can't find it.

@LLFourn
Copy link
Contributor

LLFourn commented Nov 21, 2023

Here you go:

https://github.com/rust-bitcoin/rust-miniscript/blob/c94deab5bd16b42da53ab38afcc521ae8c8bf024/src/plan.rs

It needs feedback and iterations but this is the way of the future.

@nondiremanuel nondiremanuel modified the milestones: 1.0.0-alpha.4, 1.0.0 Jan 6, 2024
@notmandatory notmandatory added module-wallet api A breaking API change labels Mar 18, 2024
@notmandatory notmandatory removed this from the 1.0.0-alpha milestone Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api A breaking API change module-wallet new feature New feature or request
Projects
Status: Todo
Development

No branches or pull requests

5 participants