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

Standalone module for creating a spend transaction #842

Merged
merged 14 commits into from
Dec 8, 2023

Conversation

darosior
Copy link
Member

Based on top of #816, this introduces a new spend module with a helper to create a transaction spending coins from the wallet. It can be leveraged to create regular or recovery transactions, and also replacement for them. All the data structures used by the exposed spend creation function are contained with this module, in order to make it usable without a lianad-specific database and Bitcoin interface.

This PR is structured in an incremental fashion. First we pull out the create_spend_internal method introduced in #816 into a standalone spend module, then we incrementally remove the cruft and the ties from the spend module to the other components.

src/spend.rs Outdated Show resolved Hide resolved
This moves create_spend_internal in bulk. The interface is still inappropriate and will be adapted in the next commits.
This is a first step toward removing the database accesses from the
spend PSBT creation helper. It now always take a change address, and
return whether it used it. If it did the caller retrieves the
information about the change address and if necessary bumps the next
derivation index to use.
Instead, pass the address details, if known, as a parameter.
@darosior darosior changed the title [PoC] Standalone module for creating a spend transaction Standalone module for creating a spend transaction Dec 7, 2023
@darosior darosior requested a review from jp1ac4 December 7, 2023 09:33
@darosior
Copy link
Member Author

darosior commented Dec 7, 2023

Rebased after #816 merge. I think this is ready for review. This is a large diff but most of it is from the move in the first commit.

Copy link
Collaborator

@jp1ac4 jp1ac4 left a comment

Choose a reason for hiding this comment

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

I reviewed f1d19f8.

It looks nice. I just found some small typos and possibly a parameter to rename.

src/spend.rs Outdated Show resolved Hide resolved
src/spend.rs Outdated Show resolved Hide resolved
src/spend.rs Outdated Show resolved Hide resolved
src/spend.rs Outdated Show resolved Hide resolved
src/spend.rs Outdated Show resolved Hide resolved
We introduce a trait to get the wallet transaction correponding to the
transaction input in order to encapsulate the spend module.
It's basically free to do now, so we might as well do it.
Now that we have sweep capability no need to duplicate half the logic, just reuse the helper.
They aren't externally called anymore
It's already performed (twice) in spend::create_psbt()
We could use a trait but instead there is just a couple fields we need so simply copy them over.
They would be discarded immediately in the filter below.
Allows for a clearer interface: you explicitly set whether you are
creating a replacement, and you don't have dangling 0s when you don't
which necessitate a comment to explain what they correspond to.
@darosior
Copy link
Member Author

darosior commented Dec 8, 2023

Since it's only a refactoring and you only found typos, i'll go ahead and merge this. I want this (and other things based on this) to land in master to be tested by others.

@darosior
Copy link
Member Author

darosior commented Dec 8, 2023

self-ACK 0f69411

@darosior darosior merged commit b5a3e78 into wizardsardine:master Dec 8, 2023
8 of 12 checks passed
@darosior darosior deleted the 2311_spend_mod branch December 8, 2023 14:10
darosior added a commit that referenced this pull request Dec 8, 2023
…and with a high number of candidates

c002ab9 spend: scale down the number of BnB when not compiling with optimizations (Antoine Poinsot)
f18086c spend: scale down the number of BnB rounds as number of candidates increases (Antoine Poinsot)

Pull request description:

  Based on #842.

  Performing BnB with a large number of candidates can be computation expensive. Especially in debug mode. It can make the software lag in situations where it's called repeatedly.
  Reasonably scale down the number of rounds performed depending on the number of candidates and whether optimizations are enabled.

  Fixes #846

ACKs for top commit:
  jp1ac4:
    ACK c002ab9.

Tree-SHA512: 83de99d54c427de86dad779549280b3316b0e72a9e3a5cb4af22d44eefa9832408b974f2a2aca0847b13ad5899e324ad43f32e99eb4efb69716c827f3ab47aaf
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.

2 participants