Skip to content

Implement version specific types #116

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GideonBature
Copy link
Contributor

@GideonBature GideonBature commented Apr 5, 2025

Implement Version-Specific Types for Status omitted

This PR addresses issue #115 by implementing the methods with status of 'omitted' in methods with version-specific types. Previously, 'omitted' was used in cases where return types were standard or seemingly inconsequential (e.g., null, standard library types). However, this approach introduced a risk: if the return type of such methods changes in future Bitcoin Core versions, we would not catch it.

To fix this, this PR:

  • Defines version-specific types for affected methods.
  • Updates method definitions to use Method::new_no_model in corresponding methods.
  • Adds integration tests for each updated method to verify correctness and catch future regressions.

This improves type safety, clarity, and future-proofing of the client interface.


Summary of Work Done

🚀 Blockchain Methods

Tested with:

cargo test --features=28_0 --test blockchain
S/N Method Name Implemented Test Written
1 pruneblockchain
2 savemempool
3 scantxoutset
4 verifychain

🌐 Network Methods

Tested with:

cargo test --features=28_0 --test network
S/N Method Name Implemented Test Written
5 addnode
6 clearbanned
7 disconnectnode
8 getconnectioncount
9 listbanned
10 ping
11 setban
12 setnetworkactive

💰 Wallet Methods

Tested with:

cargo test --features=28_0 --test wallet
S/N Method Name Implemented Test Written
13 abandontransaction
14 abortrescan
15 backupwallet
16 encryptwallet
17 importaddress
18 importmulti
19 importprivkey
20 importprunedfunds
21 importpubkey
22 importwallet
23 keypoolrefill
24 lockunspent
25 removeprunedfunds
26 sethdseed
27 settxfee
28 walletlock
29 walletpassphrase
30 walletpassphrasechange

✅ Ready for review

Closes #115

@GideonBature GideonBature changed the title WIP: feat(rpc/v17 - v28): Add initial implementation for pruneblockchain (#115) WIP: feat(rpc/v17 & v28): Add initial implementation for pruneblockchain (#115) Apr 5, 2025
Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Looking good mate. I wouldn't personally bother pruning at the moment. At this stage just checking that the returned data is the correct shape is enough.

@GideonBature
Copy link
Contributor Author

I have effected the changes reviewed, please do have a check when you are chanced.

Copy link
Member

@tcharding tcharding left a comment

Choose a reason for hiding this comment

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

Getting there man, keep at it.

@GideonBature
Copy link
Contributor Author

Changes effected... Thank you so much for all the reviews, I am really learning and understanding the codebase more.

@tcharding
Copy link
Member

I just introduced a new style of writing test code in #125. Please follow the same style when you write your new tests. Thanks man!

@GideonBature
Copy link
Contributor Author

Alright, I will check it out... Thank you :).

@GideonBature GideonBature changed the title WIP: feat(rpc/v17 & v28): Add initial implementation for pruneblockchain (#115) Implement version specific types Apr 13, 2025
@GideonBature GideonBature marked this pull request as ready for review April 21, 2025 19:48
@tcharding
Copy link
Member

tcharding commented Apr 22, 2025

You got a rebase mistake in there mate. In verify/src/method/

@tcharding
Copy link
Member

In case its useful to you I have this alias

which mc
mc: aliased to git grep -l '<<<<'

Then I use mc to check for merge conflicts after rebasing.

@tcharding
Copy link
Member

This is going to be hard for me to review because its so big but also I appreciate that it is hard to work on this crate without doing big changesets so I'm happy to review this but there may be a few iterations.

@tcharding
Copy link
Member

Some things I found:

  • Need to set the status to done e.g. types/src/v17/mod.rs when each method is added and tested
  • If you could use the new test format I'd appreciate it, otherwise I have to go over all the tests and add it
    let json: VerifyTxOutProof = node.client.verify_tx_out_proof(&proof)?;
    let model: Result<mtype::VerifyTxOutProof, _> = json.into_model();
    let txids = model.unwrap();

The point to this is to check that all the re-exports were done correctly.

Or using PruneBlockchain

    let _: PruneBlockchain = node.client.prune_blockchain(target_height);

It would probably be best to push up an initial patch that does just one method completely so I can review that and then you could do all the others as a second patch. Sorry I should have said that weeks ago, my bad. There is a way to stage code chunks so you can pull the changes out - let me know if you want more tips on that.

@tcharding
Copy link
Member

Appreciate your efforts man, this is not the easiest crate to work on. Holla at me if you have any other questions.

@GideonBature GideonBature force-pushed the omitted-types-impl branch 6 times, most recently from adeb378 to 6ac7abc Compare April 22, 2025 09:20
@GideonBature
Copy link
Contributor Author

You got a rebase mistake in there mate. In verify/src/method/

Noted, thank you for spotting it out.

@GideonBature
Copy link
Contributor Author

This is going to be hard for me to review because its so big but also I appreciate that it is hard to work on this crate without doing big changesets so I'm happy to review this but there may be a few iterations.

No Problem, I will make all changes necessary as you review! :).

@GideonBature
Copy link
Contributor Author

Some things I found:

  • Need to set the status to done e.g. types/src/v17/mod.rs when each method is added and tested
  • If you could use the new test format I'd appreciate it, otherwise I have to go over all the tests and add it
    let json: VerifyTxOutProof = node.client.verify_tx_out_proof(&proof)?;
    let model: Result<mtype::VerifyTxOutProof, _> = json.into_model();
    let txids = model.unwrap();

The point to this is to check that all the re-exports were done correctly.

Or using PruneBlockchain

    let _: PruneBlockchain = node.client.prune_blockchain(target_height);

It would probably be best to push up an initial patch that does just one method completely so I can review that and then you could do all the others as a second patch. Sorry I should have said that weeks ago, my bad. There is a way to stage code chunks so you can pull the changes out - let me know if you want more tips on that.

True, almost forgot changing their status to done.

Okayyy, now I see the reason why, for this particular methods, most of them returns null or just a standard type so I will implement for the others just like the example you provided. Thank you so much for the reviews.

I will also push up an initial patch with single method so you can review, before doing the others. I will look it up i.e. stage code chucks, and if there is anything I don't understand, I will definitely ask for clarity.

Once again thank you always :).

@GideonBature GideonBature marked this pull request as draft April 30, 2025 16:55
tcharding added a commit that referenced this pull request May 5, 2025
bbcec24 Implement pruneblock method and test (GideonBature)

Pull request description:

  Going by the conversations with tcharding for PR #116 on implementing one method at a time for easier review: This is the first method implementation `pruneblockchain` which is a specific type that returns a standard type (numeric). Once this is approved, I’ll proceed with the remaining ones.

ACKs for top commit:
  tcharding:
    ACK bbcec24

Tree-SHA512: 93b6dad6db42e6dad2c0e229553cc415f1d6c8b150dfda092f243b5ff61c7f8932878ee12d330915ca19e3b482c40d546c291692dad14980d9ac92e492f3dfbd
tcharding added a commit that referenced this pull request May 8, 2025
565702f Implement savemempool method and test (GideonBature)

Pull request description:

  Going by the conversations with tcharding  for PR #116 on implementing one method at a time for easier review: This is the second method implementation `savemempool` which is a specific type that returns a (json null). Once this is approved, I’ll proceed with the next one.

ACKs for top commit:
  tcharding:
    ACK 565702f

Tree-SHA512: 85d0d2ef8fa48cc325efc4f7eb9dfbf1276fe9185ff0c3bc2639aaec924088795638b82fb51856cd8454d24a7c59332297921ee00db38efb7e02db35bddbc1fc
tcharding added a commit that referenced this pull request May 11, 2025
ae286f1 Implement verifychain method and test (GideonBature)

Pull request description:

  This is based on PR #116 on implementing one method at a time for easier review: This is the third method implementation: `verifychain` which returns a (bool type). Once this is approved, I’ll proceed with the next one.

ACKs for top commit:
  tcharding:
    ACK ae286f1

Tree-SHA512: 9992984539ead8a63b394b431df0b52cf99d552d17c3c4930aa406660e1b51b5e43fc07e5bfeda1f7c9c0234278d212d42e2f893b97a7de1e208199bc8012aca
tcharding added a commit to tcharding/corepc that referenced this pull request May 15, 2025
The `ScriptPubkey` type is a type that is returned by Core in various
places. It seems like the Core devs put a fair bit of effort into
it (probably because of this) and the when the type changed in v22 it
was changed in a way that was completely backwards compatible.

I failed to fully understand this when first adding the type.

With this patch applied there is now only one `ScriptPubkey` type (in
`libr.s`) and also we add an model type.

The `ScriptPubkey` type is unusual in that its fields can be used to
create `TxOut` if tx value is known - which for example in `GetTxOut` it
is.

So here we also remove the v22 `GetTxOut` type since it only existed
because of the perceived difference in `ScriptPubkey` which no longer
exists.

Fix: rust-bitcoin#116
tcharding added a commit to tcharding/corepc that referenced this pull request May 15, 2025
The `ScriptPubkey` type is a type that is returned by Core in various
places. It seems like the Core devs put a fair bit of effort into
it (probably because of this) and when the type changed in v22 it was
changed in a way that was completely backwards compatible.

I failed to fully understand this when first adding the type.

With this patch applied there is now only one `ScriptPubkey` type (in
`libr.s`) and also we add a model type.

The `ScriptPubkey` type is unusual in that its fields can be used to
create `TxOut` if tx value is known - which for example in `GetTxOut` it
is.

So here we also remove the v22 `GetTxOut` type since it only existed
because of the perceived difference in `ScriptPubkey` which no longer
exists.

Fix: rust-bitcoin#116
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.

Implement version specific types for status 'omitted' methods
2 participants