Skip to content

Conversation

@tcharding
Copy link
Member

@tcharding tcharding commented Nov 5, 2025

This is not a merge candidate.

Done initially with claude, then I rebased and pulled things apart and added a bunch more patches. The two patches done with claude are the ugrade and the test fix. Although when rebasing I'm not 100% sure I did not introduce changes to claude's original work.

Before this can pass all the integration tests in CI we have to upgrade corepc-node ...

@tcharding
Copy link
Member Author

tcharding commented Nov 5, 2025

cc LLFourn because he got me started on Claude.

EDIT: Removed mention of Lloyd, that horse has bolted.

@apoelstra
Copy link
Member

Whew. I wonder if we can update secp256k1 first.

@tcharding
Copy link
Member Author

ooo, that's a nice idea. I'm keen for Saturday morning miniscript so I"ll let Claude spin on a new go from scratch just updating secp, have a cup of tea, and review this PR for giggles.

@tcharding
Copy link
Member Author

tcharding commented Nov 8, 2025

I didn't end up exploring the upgrade-secp-first-route. I started but stopped, now I think more I'd like to try:

  1. patch miniscript to use scep directly instead of the bitcoin re-export
  2. upgrade secp to 0.30
  3. upgrade secp to 0.31

This would mean this PR becomes void, I personally do not want to run claude again to do the upgrade because it took me 5 days and was boring as hell. Still looking for another way to progress. FTR I spent a fair bit of time reviewing this and still didn't get all the way through but here is what I found:

# Notes for upgrading to bitcoin 0.33.0-beta.0

The non-trivial things are:

- script tagging
- secp context removal

Kind of trivial things:

- input/output -> inputs/outputs
- secp Into<Message> thing, causes addition/removal of `*` and `&`
- from_ apis have changed to more idiomatic names
- Default removed
- txout.value -> txout.amount


Things now disallowed on purpose:

- The hashes wrapper types cannot hash arbitrary data so there is a
  bunch of casting required.

## Thing to improve the CLAUDE.md file with

- Only fix build errors not warnings (eg don't fix warnings about
  deprecated function calls).
- Use `Amount::from_sat_u32` instead of `Amount::from_sat().unwrap()`
- Use the `bitcoin::XOnlyPublicKey` API instead of creating a secp key and calling `into`.
- Claude uses `let push_bytes = <&PushBytes>::try_from(b); push_bytes.read_scriptint()` instead of discovering `b.read_scriptint`

## Things claude did well

- using `_` to rename secp context function paramater instead of removing it
- Worked out which associated consts to use instead of `default`

### Script tagging questions

- Do we want to favour one script type over the others in test code?
- Basically anything that does not have a local varibale name of
  function name that hints at the type needs checking by Andrew (or
  Tobin needs to upgrade his bitcoin script knowledge - or both).

- Is script code a `ScriptPubKey` some other type?

## Bitcoin API questions

- Does the `XOnlyPublicKey` API have holes in it?
- Are we happy with users having to reach into `bitcoin::script` to
  get the new script types.
  
## Things I did not look too closely at - assuming they are correct

- error type changes

@tcharding
Copy link
Member Author

I've done reviewing now. I found the second review session today quiet educational and even peaceful.

Final notes:

  • There are some issues around use of the new bitcoin::XOnlyPublicKey but I don't know if this is caused by API holes or by needing to refactor here to use it everywhere instead of the secp one (or vice versa).
  • I think all the hash casting is as expected?
  • I learned a bit more about script, script code is incorrectly handled here (I think), but because of legacy I'm not sure about the solution.
  • I guess it should not be surprising that this upgrade is hard in this crate, other users hopefully do not use scripts so heavily?? Otherwise we are going to get some complaints, but this one is worth defending, right?

Use a patched version of bitcoin 0.32 that adds the `XOnlyPublicKey`
that is currently on master.

Done to reduce the size of the diff when upgrading `bitcoin` to the
upcoming RC release.
FYI claude left the repo with a broken build. I fixed the build as a
separate patch for reasons listed below.

First I got claude to create a CLAUDE.md file. Then I pointed it at:

- A local `rust-bitcoin` directory on the release tracking branch
    rust-bitcoin/rust-bitcoin#5169
- The docs online for `bitcoin v0.32.7`
- The `rust-miniscript` repo

And asked it to do the upgrade. It took me three goes over the weekend
to do this, hitting session limits twice.

I am pushing the raw changes Claude made so:

- I can review them in public
- We can learn how good/bad Claude is for doing upgrades
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