Upgrade bitcoin to the upcoming 0.32.0-rc.0 release#874
Upgrade bitcoin to the upcoming 0.32.0-rc.0 release#874tcharding wants to merge 17 commits intorust-bitcoin:masterfrom
bitcoin to the upcoming 0.32.0-rc.0 release#874Conversation
|
cc LLFourn because he got me started on Claude. EDIT: Removed mention of Lloyd, that horse has bolted. |
|
Whew. I wonder if we can update |
|
ooo, that's a nice idea. I'm keen for Saturday morning |
c14c19b to
b43197b
Compare
|
I didn't end up exploring the upgrade-secp-first-route. I started but stopped, now I think more I'd like to try:
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
|
b43197b to
d16486c
Compare
|
I've done reviewing now. I found the second review session today quiet educational and even peaceful. Final notes:
|
c660dbb to
cdb6926
Compare
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.
This leaves the test failing. Fixed next. I originally did this with claude but claude left the build broken, also I rebased a few weeks later which require a bunch more manual changes.
This patch got mixed up along the way ...
cdb6926 to
137c803
Compare
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...To develop this I used:
bitcoincorepc#528