-
Notifications
You must be signed in to change notification settings - Fork 27
Add support for bitcoin core 29.0 #131
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
base: master
Are you sure you want to change the base?
Conversation
5438355
to
1dbf4df
Compare
CI fail can be fixed by creating the RPC help file, I haven't downloaded Core v29 yet but I'd expect that on this branch we could do: contrib/run-bitcoind.sh start v29
bt29 help > verify/rpc-api-v29.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Man this is looking pretty good! This crate is hard to work on so don't feel bad if you miss things. I'm having a hard time reviewing because the changeset is so big but I've left a few comments that will make it a fair bit smaller. Keep at it! Appreciate your effort.
I tried running the command after starting the bitcoind v29, but it keeps say the |
dc856f3
to
3c6c22f
Compare
Oh I have a shell alias for each version, for v28 I have bt28: aliased to bitcoin-cli -rpcconnect=localhost:28049 -rpcuser=*** -rpcpassword=*** And fill in the stars for your setup. Remember the leading 280 comes from the config file of |
3c6c22f
to
8135a58
Compare
Thank you... It's done! |
798a62e
to
2308f10
Compare
20080f5
to
87a78dd
Compare
Can you get rid of all the formatting changes please. |
fb921bd
to
5709498
Compare
Done! |
fa9350a
to
1db66bf
Compare
It is surprising to see changes to |
Alright, so for v29, the |
Yes please. Anything that is different in one version to another should have a separate type, errors included. I'm not totally convince that we should re-use errors at all but we already do in submitpackage IIRC and its kinda nice. |
Alright, noted, I am going to do that right away... |
8341786
to
717ab4d
Compare
No changes to v17 in a PR that adds support for v29 please. EDIT: This should have been 'no changes to v17 types', I'd expect changes to the |
9a1982e
to
a2840af
Compare
Noted. I have removed the changes I made to the v17 and others. I will open a separate PR to address those. I removed all changes made to the v17 and other versions other than v29 with this recent push. I also reduce the docs on the getdescriptoractivity struct. |
We need to update Run |
There is still changes in |
oh my... fixing that. |
3180c5c
to
124e08a
Compare
For this recent push, I removed all the unnecessary code in the client, made changes to the methods in the verify module of v29 to march the correct ones. |
"29 checks for v29" :). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting there, keep at it man.
let blockhashes_val = json!(blockhashes); | ||
let scan_objects_val = json!(scan_objects); | ||
let include_mempool_val = json!(include_mempool); | ||
|
||
let params = vec![blockhashes_val, scan_objects_val, include_mempool_val]; | ||
|
||
self.call("getdescriptoractivity", ¶ms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let blockhashes_val = json!(blockhashes); | |
let scan_objects_val = json!(scan_objects); | |
let include_mempool_val = json!(include_mempool); | |
let params = vec![blockhashes_val, scan_objects_val, include_mempool_val]; | |
self.call("getdescriptoractivity", ¶ms) | |
self.call("getdescriptoractivity", &[]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I do this, the method won't work, because all the parameters are required even if they contain no value, they are to be present as empty arrays, including the bool value. Running the test, we will always have this:
getdescriptoractivity: JsonRpc(Rpc(RpcError { code: -3, message: "JSON value of type null is not of expected type array", data: None }))
That is the reason why I had to be passing an empty array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is odd, the docs say they are optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, while working on it at first, I tried without including the parameters, and I kept getting this error while running the test. So I decided to try by passing in an empty array instead, only then did it work.
/// The bits. | ||
pub bits: String, | ||
/// The difficulty target. | ||
pub target: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You marked this resolved but its not resolved.
types/src/model/blockchain.rs
Outdated
/// The bits | ||
// pub bits: Option<CompactTarget>, // Only from v29 onwards | ||
/// The difficulty target. | ||
// pub target: Option<Target>, // Only from v29 onwards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I commented it out because it's going to cause errors in v17 and v28 implementation of the into method. Which I will need to resolve. Since we are not supposed to touch the other methods in this issue, I intend to work on these in a different PR.
I've a bad feeling now that I told you then wrong thing before, if so apologies for that. |
You've put a fair bit of effort into this and I feel bad for giving the dud review. Do you want me to help you push it over the line i.e., want me to make the final changes and push them up? Totally up to you, I'm good to wait and just review also. |
No problem, I am comfortable you reviewing it. I am really okay with that. :). |
No problem. Please don't feel bad, maintaining this library is not easy, as sometimes making changes to a file also affect others, so I feel sometimes missing somethings in that case is normal. |
124e08a
to
3e54aff
Compare
/// The bits | ||
pub bits: Option<CompactTarget>, // Only from v29 onwards | ||
/// The difficulty target. | ||
pub target: Option<Target>, // Only from v29 onwards |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate above.
Man it looks like you haven't rebased this for a while. You likely need to rebase on master then check a bunch of files using |
seems I had a mix up somewhere. Thank you for pointing these out. |
3e54aff
to
521f814
Compare
The Most recent changes I made was to resolve pending issues, despite I rebase master earlier, seems somethings didn't apply the way they're supposed to, took care of that also. Thank you! |
521f814
to
b46666e
Compare
b46666e
to
94554b6
Compare
This PR adds support for the newly released bitcoin core version - v29.0
Updated RPCs
getmininginfo
which now returns nBits and the current target in the target field. It also returns a next object which specifies the height, nBits, difficulty, and target for the next block. (test added)getblock
andgetblockheader
which now return the current target in the target field (test added)getblockchaininfo
andgetchainstates
which now return nBits and the current target in the target field. (test added)getblocktemplate
whose RPC curtime (BIP22) and mintime (BIP23) fields now account for the timewarp fix proposed in BIP94 on all networks. (test added)New RPCs
getdescriptoractivity
rpc method which can be used to find all spend/receive activity relevant to a given set of descriptors within a set of specified blocks. (test added)Closes #129