blockchain, btcjson: Implement getchaintips rpc call #1918
Conversation
kcalvinalvin
force-pushed
the
2022-11-06-implement-getchaintips
branch
from
a75d949
to
861aebb
Compare
Pull Request Test Coverage Report for Build 5566378590
💛 - Coveralls |
kcalvinalvin
force-pushed
the
2022-11-06-implement-getchaintips
branch
from
861aebb
to
eee2537
Compare
|
I'm not familiar with the btcd process for this: How/when is json_rpc_api.md updated? Automatically when doing a release or manually in PRs? In case of the latter, adding RPC docs here would be good. |
btcjson/chainsvrresults.go
Outdated
| // GetChainTipsResult models the data from the getchaintips command. | ||
| type GetChainTipsResult struct { | ||
| ChainTips []ChainTip `json:"chaintips"` | ||
| } |
There was a problem hiding this comment.
I'm not sure how closely you want to follow the Bitcoin Core RPC response. From the docs I got the impressions that some RPC calls are/were compatible with Bitcoin Core.
Bitcoin Core's getchaintips RPC returns a JSON list of chain tips as top level element:
[
{
"height": 115861,
"hash":" 000000407476239a4870e510ce1dea262c9afa672eae3ab50a019812f83e0059",
"branchlen": 0,
"status": "active"
}
]
This currently returns a JSON map with a single key called chaintips and the list of chain tips as value.
"chaintips": [
{
"height": 115861,
"hash":" 000000407476239a4870e510ce1dea262c9afa672eae3ab50a019812f83e0059",
"branchlen": 0,
"status": "active"
}
]
There was a problem hiding this comment.
Changed the output to match that of Bitcoin Core
By the looks of it from this commit a6c79c7, seems like you need to manually update the log when you implement the rpc call. I'll update the file. |
kcalvinalvin
force-pushed
the
2022-11-06-implement-getchaintips
branch
2 times, most recently
from
b42df7f
to
566b618
Compare
|
Updated |
kcalvinalvin
changed the title
There was a problem hiding this comment.
Thanks for picking this up!
One thing I'd like to get back in the habit of is adding more rpctests. This would let us exercise the new RPC interface a bit more by doing things like mining invalid blocks, creating forks, etc -- then asserting that the final output is what we'd expect.
blockchain/chain.go
Outdated
| Height int32 | ||
| // BlockHash hash of the tip. | ||
| BlockHash chainhash.Hash | ||
| // Amount of blocks connecting this tip with the best chain. Returns |
There was a problem hiding this comment.
A bit confused re the wording here: this is meant to the the height gap between this non-main chain tip and the main chain itself?
There was a problem hiding this comment.
It's meant as the amount of blocks the branch has from the oldest common ancestor with the active chain. Active chain always returns 0.
The below branch of "unknown" would have a branchlen of 2 since it forks off at block 10 and has 2 blocks built on top of block 10.
// genesis -> 1 -> 2 -> 3 ... -> 10 -> 11 -> 12 -> 13 (active)
// \-> 11a -> 12a (unknown)
There was a problem hiding this comment.
Makes a lot of sense, I think we can add some more of this context to the comment itself (can even add that diagram above).
kcalvinalvin
force-pushed
the
2022-11-06-implement-getchaintips
branch
from
566b618
to
d3086cd
Compare
|
Addressed all of the modifications requested. I'm not too familiar with how Edit: From a glance the existing rpc test look like the functional tests in core. Maybe importing the functional tests from core is something that could be desirable in the future? |
|
Added a new commit that adds a test for the |
Yeah they're similar-ish: main idea is to be able to drive nodes over RPC to trigger certain p2p/mempool/chain behavior. Re just porting over the Bitcoin Core functional tests: we ended up going this route so we'd only have a single language in the repo (just Go). If we started to port their tests directly, then we'd also need to ensure a Python CI env and the proper docs to get things up and running. Over time though, our RPC interface has started to drift away from theirs, so there would be certain tests that just wouldn't be compatible at all. Nevertheless, it'd basically be test coverage we'd get for free, so it's def worth looking into. |
| // of the orphans. | ||
| orphans := make(map[chainhash.Hash]*blockNode) | ||
| orphanParent := make(map[chainhash.Hash]*blockNode) | ||
| for hash, node := range bi.index { |
There was a problem hiding this comment.
Seeing index interactions like this makes we wish we had that skip list structure integrated 🤓
There was a problem hiding this comment.
Ah yeah I had a PR open for that a long time ago. Should clean that up too.
blockchain/chain.go
Outdated
| // BlockHash hash of the tip. | ||
| BlockHash chainhash.Hash | ||
|
|
||
| // BranchLen is the amount of blocks connecting this tip with the best chain. |
There was a problem hiding this comment.
Potential comment reframing:
BranchLen is length of the fork point of this chain from the main chain
?
|
|
||
| chainTips := make([]ChainTip, 0, len(tips)) | ||
|
|
||
| // Go through all the tips and grab the height, hash, branch length, and the block |
There was a problem hiding this comment.
All of the comments below are under 80 column though (with 8 space tabs). Could you point out which ones should be wrapped?
blockchain/chain.go
Outdated
| // status. | ||
| for _, tip := range tips { | ||
| var status TipStatus | ||
| if b.bestChain.Contains(tip) { |
There was a problem hiding this comment.
Potentially more readable as a switch statement (then the comments can go right above where the case lies).
| } | ||
|
|
||
| for _, test := range tests { | ||
| chain, expectedChainTips := test.chainTipGen() |
There was a problem hiding this comment.
Same comment re line folding below.
There was a problem hiding this comment.
Is there a desirable column width limit? There's many parts of btcd code that's over 100.
(My personal taste is that anything under 110 is acceptable)
integration/getchaintips_test.go
Outdated
| return fmt.Errorf("Couldn't find expected chaintip with hash %s", expectedChainTip.Hash) | ||
| } | ||
|
|
||
| err := compareChainTips(gotChainTip, *expectedChainTip) |
There was a problem hiding this comment.
Alternatively can just use something like require.Equal, it'll print out where the diff is, vs needing to scan thru to find the field, and also if more fields are added, it won't need to be modified.
| func TestGetChainTips(t *testing.T) { | ||
| // block1Hex is a block that builds on top of the regtest genesis block. | ||
| // Has blockhash of "36c056247e8c0589f6307995e4e13acf2b2b79cad9ecd5a4eeab2131ed0ecde5". | ||
| block1Hex := "0000002006226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf18891" + |
There was a problem hiding this comment.
Move to the top of the file as constants?
There was a problem hiding this comment.
Alternatively, we could also just generate these blocks. This method would let us mine empty blocks at will. The test would then use a series of miners (connect, mine block, disconnect, let it mine its own blocks, etc) to dynamically create the chain.
There was a problem hiding this comment.
Alternatively, we could also just generate these blocks.
We could and that'd be preferable. But at the moment there's no support for invalidateblock so we can't test for reorgs.
A timeline like below would work:
Get this PR in -> get invalidateblock in -> change tests to generate blocks
| // Our chain view looks like so: | ||
| // (genesis block) -> 1 -> 2 -> 3 -> 4 | ||
| blockStrings := []string{block1Hex, block2Hex, block3Hex, block4Hex} | ||
| for _, blockString := range blockStrings { |
There was a problem hiding this comment.
So with the comment above, these would just be mined w/ empty params (will auto use the prior to build off of).
There was a problem hiding this comment.
Yup. But as mentioned in here, we can't do that quite yet.
|
|
||
| // Submit a single block that builds on top of 3a. | ||
| // | ||
| // Our chain view looks like so: |
| BranchLen: 1, | ||
| Status: "valid-fork", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Looks like there's a missing call to compareMultipleChainTips here (lack of assertion for final case).
|
Interested in finishing this up for it to land? |
InactiveTips() returns all the tips of the branches of the blockchain tree that are not in the best chain. This function is useful for supporting the getchaintips rpc call.
ChainTips method allows for callers to get all the chain tips the node is aware of. This is useful for supporting the getchaintips rpc call.
getchaintips call is implemented and the behavior mimics that of Bitcoin Core. Resolves btcsuite#1912.
rpcclient now support calling the getchaintips rpc call. getchaintips_test.go adds test for the getchaintips rpc call. The test includes hard-coded blocks which the test will feed the node via rpc and it'll check that the returned chain tips from the getchaintips call are chaintips that we expect to be returned.
kcalvinalvin
force-pushed
the
2022-11-06-implement-getchaintips
branch
from
5924bb4
to
fc99e96
Compare
|
Addressed most things in the review and asked for clarification on the rest |
|
Hi @Roasbeef this pr is also ready for your next round of review |
getchaintips rpc call is implemented and follows the algorithm that Bitcoin Core has in https://github.com/bitcoin/bitcoin/blob/50422b770a40f5fa964201d1e99fd6b5dc1653ca/src/rpc/blockchain.cpp#L1374.
Test coverage checks that the height, block hash, branchlen, and status are correct. I also tested the rpc on regtest with blocks:
78b945a390c561cf8b9ccf0598be15d7d85c67022bf71083c0b0bd8042fc30d71b97df2be6c634e40a5f410f4758ba707c4452cda6f3707b4935c9e7760df965which both build on top of the genesis block.
raw blocks:
block:
716c71f0c4990732b51dc611825fdd11fc8dc471ffc3e5c1de86b5d6e4663ff2builds on top of block78b945a390c561cf8b9ccf0598be15d7d85c67022bf71083c0b0bd8042fc30d7and it was used to test reorging and making sure getchaintips behavior matches that of bitcoin core.raw block:
Resolves #1912