Skip to content

Prepare Plan::update_psbt_input to migration to rust-psbt#974

Open
nymius wants to merge 2 commits into
rust-bitcoin:masterfrom
nymius:push-ysxrnpmmmtvq
Open

Prepare Plan::update_psbt_input to migration to rust-psbt#974
nymius wants to merge 2 commits into
rust-bitcoin:masterfrom
nymius:push-ysxrnpmmmtvq

Conversation

@nymius
Copy link
Copy Markdown
Contributor

@nymius nymius commented Jun 2, 2026

This is the simplest way I found to use the public Plan API in the Plan::update_psbt_input method so It can later be migrated to the rust-psbt project.

Notes to the reviewers:

  • There is any reason to not expose the descriptor and template fields directly?
  • There is a TODO in the code, but I'm lacking the context of it. It is something that should be fixed or a leftover?

@apoelstra
Copy link
Copy Markdown
Member

  • There is any reason to not expose the descriptor and template fields directly?

Nope, go for it.

  • There is a TODO in the code, but I'm lacking the context of it. It is something that should be fixed or a leftover?

The context appears to be a few lines below where we do a loop for (pk, key_source) in data.key_origins which then builds a new tree in input.tap_key_origins. I think the TODO is that afillini imagined that he could somehow directly memcpy the tree structure over instead of doing a tree traversal?

Probably fine to delete the TODO. It dates to #592, like much of the "weird" stuff in this module, and likely wasn't reviewed since the new module was too big.

@nymius nymius force-pushed the push-ysxrnpmmmtvq branch 2 times, most recently from 5c6ed14 to eeeeda9 Compare June 2, 2026 18:54
@nymius
Copy link
Copy Markdown
Contributor Author

nymius commented Jun 2, 2026

Thanks for clarifying _@apoelstra , I've removed the TODO as the functionality seems covered and the possible explanation is related to a performance improvement.

I've publicly exposed the descriptor field as it was the only field that I didn't have access to.

As removing the witness_template method would be a breaking change, and exposing the template field is duplicating the functionality provided by this method, I'm leaving as it.

I've copied the code from here into rust-psbt to verify it helps moving towards removing PSBT stuff from miniscript. You can check it in: https://git.rust-bitcoin.org/nymius/rust-psbt/compare/push-yormpqokyrxz...nymius/rust-psbt:push-xstopvrvruzp

@nymius nymius force-pushed the push-ysxrnpmmmtvq branch from eeeeda9 to 0183420 Compare June 5, 2026 14:37
@nymius
Copy link
Copy Markdown
Contributor Author

nymius commented Jun 5, 2026

I've removed the changes that weren't needed by rust-psbt to port update_psbt_input there.

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