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

Improving Plan API and adding examples. #559

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Harshil-Jani
Copy link
Contributor

@Harshil-Jani Harshil-Jani commented Jun 30, 2023

This PR intends to complete some after works on top of #481 . Me and @sanket1729 had multiple iterations since I was working on it as a Summer of Bitcoin project.

The goals which we have added for improvement over the plan module are as follows :

  • Adding a method to get all the possible spend paths for a given descriptor.
  • Counting totals number of possible spend path
  • Keeping a threshold of about 1000 spend path so no large descriptor can ever be computed. This prevents DoS Attack.
  • Updating PSBT examples in order to explain the end outcome for the plan API and choosing spend path based on descriptor.

@sanket1729
Copy link
Member

You would need cargo +nightly fmt to make this work.

Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small comments

examples/psbt_sign_finalize.rs Outdated Show resolved Hide resolved
examples/psbt_sign_finalize.rs Show resolved Hide resolved
examples/psbt_sign_finalize.rs Outdated Show resolved Hide resolved
@tcharding
Copy link
Member

This is pretty hard to review because there are a load of patches that are not related to the PR description that make non-trivial changes. Perphaps either push them up as separate PRs or add to the description something like "WIP, only review the last patch please, still work in progress" if that is what is intended. Feel free to say if I've totally missed some context and it should be clear what is going on here.

@Harshil-Jani Harshil-Jani changed the title Adding examples to explain the planner API WIP : Plan API improvements and explaining the use in examples Aug 2, 2023
@Harshil-Jani
Copy link
Contributor Author

This is pretty hard to review because there are a load of patches that are not related to the PR description that make non-trivial changes. Perphaps either push them up as separate PRs or add to the description something like "WIP, only review the last patch please, still work in progress" if that is what is intended. Feel free to say if I've totally missed some context and it should be clear what is going on here.

Sorry for not adding enough context. I have added some context about the work in the PR description. The PR currently depends on plan API and we are waiting for it to get merged. For reviewing, I would say https://github.com/rust-bitcoin/rust-miniscript/pull/559/files/5166d85171c3561c2acaa99c175c08e2a9b1164e..bc99df24fe269a4f64c9136243ab4046d0cda533#diff-4e793d33eeb8ca8c2f0404fe38b64bb60e0c7bb596977b5619777ed59213978a
These commits are the core logic. For other commits , I can raise a different PR and few of it is being already address in #481.

@Harshil-Jani Harshil-Jani force-pushed the plan_spend_example_branch branch 3 times, most recently from 561a883 to 61ee4f7 Compare August 7, 2023 16:40
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some feedback after one round of review.

src/miniscript/astelem.rs Outdated Show resolved Hide resolved
src/miniscript/astelem.rs Outdated Show resolved Hide resolved
src/miniscript/astelem.rs Outdated Show resolved Hide resolved
src/descriptor/mod.rs Outdated Show resolved Hide resolved
src/descriptor/mod.rs Outdated Show resolved Hide resolved
@@ -569,6 +569,51 @@ impl Descriptor<DefiniteDescriptorKey> {
}

impl Descriptor<DescriptorPublicKey> {
/// Count total possible assets for a given descriptor.
pub fn count_assets(&self) -> u64 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that this function will not overflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. It may overflow. I can have some checks over the overflow conditions.

src/descriptor/mod.rs Outdated Show resolved Hide resolved
src/miniscript/astelem.rs Outdated Show resolved Hide resolved
src/miniscript/astelem.rs Outdated Show resolved Hide resolved
examples/plan_spend.rs Outdated Show resolved Hide resolved
@Harshil-Jani Harshil-Jani force-pushed the plan_spend_example_branch branch 3 times, most recently from 25f8bf5 to cfbd65a Compare August 16, 2023 21:33
@Harshil-Jani Harshil-Jani force-pushed the plan_spend_example_branch branch 2 times, most recently from a3de3a3 to bee48ff Compare September 25, 2023 13:19
@Harshil-Jani Harshil-Jani marked this pull request as ready for review September 25, 2023 13:38
@Harshil-Jani Harshil-Jani changed the title WIP : Plan API improvements and explaining the use in examples Plan API improvements and explaining the use in examples Sep 25, 2023
@tcharding
Copy link
Member

Hi @Harshil-Jani, good work splitting the PR up and organising your commits. Here is a blog post that I link every apprentice programmer to so they can learn to write commit messages that help them get their commits merged. The easier you make it for reviewers to review your work the more chance you have of them doing so and of you hard work getting merged. Review time is the by far the most limited resource we have in open source, the better we all make use of it the better for the project. Thanks mate. (Also you can update your PR description now this is off draft.)

https://cbea.ms/git-commit/

@Harshil-Jani Harshil-Jani changed the title Plan API improvements and explaining the use in examples Improving Plan API and adding examples. Sep 27, 2023
@Harshil-Jani
Copy link
Contributor Author

Hi @Harshil-Jani, good work splitting the PR up and organising your commits. Here is a blog post that I link every apprentice programmer to so they can learn to write commit messages that help them get their commits merged. The easier you make it for reviewers to review your work the more chance you have of them doing so and of you hard work getting merged. Review time is the by far the most limited resource we have in open source, the better we all make use of it the better for the project. Thanks mate. (Also you can update your PR description now this is off draft.)

https://cbea.ms/git-commit/

@tcharding Thanks a lot for the link. I have taken your feedback and improved my commit messages aliging with the diff. Would be awaiting for review.

@tcharding
Copy link
Member

tcharding commented Oct 3, 2023

Thanks for the extra effort @Harshil-Jani, could you please add the "why" to the commit log of each patch - its really hard to review changes if one doesn't know why the author is making the changes.

https://cbea.ms/git-commit/#why-not-how

Using `all_assets` method, you can get all possible asset for a given
node of Miniscript AST. Eg : multi(2,A,B,C) will give possible
combinations as AB, AC, BC.

This API will allow the developers to figure out all the possible spend
paths for a given descriptor.

Signed-off-by: Harshil Jani <[email protected]>
This commit wraps the all_assets function for all possible descriptor
types. It also adds `count_assets` function which can count the total
possible ways to obtain an asset for a given descriptor.

This API will help developer to count the possible ways. In-fact this
will also help in protecting that if some descriptor is very large and
has huge number of possible spend paths then it could simply be
dropped off and we can protect the network from traffic.

Signed-off-by: Harshil Jani <[email protected]>
There were helper functions to compute nCk (k combinations of n given
things) and product of K different combinations. Those were duplicated
in mod.rs of descriptor and astelem.rs of miniscript. This commit
de-duplicates them and store them in util.rs

Signed-off-by: Harshil Jani <[email protected]>
For the planning module we are considering that total possible ways to
spend should always be less than 1000. This protects the network from
any DDoS Attack if in case you receive a very large descriptor enough
that it has 1000 possible spend paths.

Signed-off-by: Harshil Jani <[email protected]>
This commit adds a new example file which explains how we can use
planner API with the taproot descriptor and compute different possible
spend path and actually spend them.

This would help developers to understand and learn new planning functionality.

Signed-off-by: Harshil Jani <[email protected]>
Recently we have had plan API into the rust-miniscript so this example
is being updated for better use of plan API with more clear explanation

Signed-off-by: Harshil Jani <[email protected]>
@Harshil-Jani
Copy link
Contributor Author

@tcharding Sorry for a long delay. Had some personal stuff came up. I have again updated the commit message. Please check them out and let me know in case those are unclear.

plan.update_psbt_input(&mut input);

// Update the PSBT input from the result which we have obtained from the Plan.
input.update_with_descriptor_unchecked(&descriptor).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we update the input with the descriptor after updating it with the plan. The plan should do everything you need?

Copy link
Contributor Author

@Harshil-Jani Harshil-Jani Nov 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While finalizing we need to inform the signer which keys we want to sign. And to do this we need pkh in place of their hashes.

The way current planner module is designed it doesn't support raw pkh and converts things into hashes. So, there is this workaround #589 and #557 which substitutes pkh from expr_raw_pkh.

Here what we did is that we took all the possible keys and mapped them with their hashes only to convert them back to their pkh form. Assuming that we only depend on plan then it is going to return us with the hash of keys that are required to be signed. But we would also need other pkh's that we need to dissatisfy.

Reference to more context on this :
#481 (comment)
#481 (comment)
#589
#557

Example of above discussion

Assume five keys A,B,C,D,E
User wants to plan txn using with A,C,E.

Case 1 : We don't update the input with descriptor

  • Plan returns Hash(A), Hash(C), Hash(E)
  • Input only contains hashes for A,C,E
  • Map contains : | A, Hash(A) | C, Hash(C) | E, Hash(E) |
  • We don't get pkhs for B and D since they are not present in the map.

Case 2 : We update the input with descriptor

  • Plan returns Hash(A), Hash(C), Hash(E)
  • We update input with descriptor so it contains A,B,C,D,E
  • Map contains : | A, Hash(A) | B, Hash(B) | C, Hash(C) | D, Hash(D) | E, Hash(E) |
  • We have all pkh full keys.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants