Feat/appchain network improvements #2862
Conversation
…d of write_next() for byte arrays (since they're not actually arrays in this case)
…le to stream back a list of Stacks block headers, in service of the new /v2/headers RPC endpoint
…ndexing at a particular sortition height, even if the downloader isn't idle.
…to sync with at the end of an inv pass
… vars, and remove some dead test code
…r data we don't have, and if the P2P state-machine is doing an inv-sync, then *immediately* stop the inv sync, switch to the downloader step, and point the downloader to the sortition of the block or microblock that is now available. This makes the p2p network much more aggressive about fetching newly-arrived blocks and confirmed microblock streams
…) as well as Stacks headers
…nd Stacks headers
…ata capable of representing a headers stream
…references to appchains
jcnelson
requested review from
lgalabru,
reedrosenbluth,
kantai,
pavitthrap and
gregorycoppola
| @@ -894,8 +894,10 @@ impl BlockDownloader { | |||
| &mut self, | |||
| block_sortition_height: u64, | |||
| ibd: bool, | |||
| force: bool, | |||
There was a problem hiding this comment.
Not super clear what force is to me - perhaps add comment?
src/net/p2p.rs
Outdated
| if inv.block_bitvec.len() > 0 { | ||
| inv.block_bitvec[0] == 0 | ||
| } else { | ||
| false |
There was a problem hiding this comment.
Could add method to BlocksInvData for this (suggestion applies to the microblock fn below)
src/net/rpc.rs
Outdated
| ref mut convo_client, | ||
| ref mut peer_server, | ||
| ref mut convo_server| { | ||
| let principal = |
There was a problem hiding this comment.
Not using principal in function? Same for test below
|
|
||
| #[test] | ||
| #[ignore] | ||
| fn test_rpc_get_data_var_unconfirmed() { |
There was a problem hiding this comment.
Perhaps could add a test trying to retrieve a var that does not exist?
src/chainstate/stacks/db/blocks.rs
Outdated
| .unwrap(); | ||
| headers.push(next_header); | ||
| } | ||
| if header_ptr.len() == 0 { |
There was a problem hiding this comment.
Should there be a similar check before the loop (for the case where header_bytes has length 0)?
There was a problem hiding this comment.
This code is not called anywhere anymore, so I'll drop it.
| @@ -77,6 +77,96 @@ Reason types without additional information will not have a | |||
|
|
|||
| Get current PoX-relevant information. See OpenAPI [spec](./rpc/openapi.yaml) for details. | |||
|
|
|||
| ### GET /v2/headers/[Count] | |||
There was a problem hiding this comment.
It'd be better for the new endpoints to be documented in the OpenAPI spec (./rpc/openapi.yaml). That gets used for autogenerating API docs in a couple different places, but also is used by the stacks-blockchain-api to do schema validation.
When we adding the openapi.yaml spec to this repo, we left this doc in place without a plan for deprecating it, however, I think it only makes sense to have one specification, and of the two, I'd prefer the OpenAPI spec.
There was a problem hiding this comment.
Damn, forgot to do this. Will send a separate PR.
… that the header is a hex string but the struct is a JSON object
… sync can terminate
…) instead of clarity_db.get_with_proof() if the caller did not ask for a proof
Codecov Report
@@ Coverage Diff @@
## develop #2862 +/- ##
===========================================
- Coverage 82.88% 82.15% -0.74%
===========================================
Files 235 235
Lines 189222 189697 +475
===========================================
- Hits 156840 155837 -1003
- Misses 32382 33860 +1478
Continue to review full report at Codecov.
|
…then still return an empty proof (this is consistent with the current behavior)
src/blockstack_cli.rs
Outdated
| const DECODE_HEADER_USAGE: &str = "blockstack-cli (options) decode-header [block-path-or-stdin] | ||
|
|
||
| The decode-header command decodes a serialized Stacks header and prints it to stdout as JSON. | ||
| The header, if given, must be a hex string. Alternatively, you may pass - instead, and the |
There was a problem hiding this comment.
Back quoting - will make the last sentence more clear.
| let block_header: StacksBlockHeader = StacksChainState::consensus_load(block_path)?; | ||
| Ok(Some(block_header)) | ||
| } | ||
|
|
||
| /// Load up an anchored block header from the chunk store. | ||
| /// Returns Ok(Some(blockheader)) if found. | ||
| /// Returns Ok(None) if this block was found, but is known to be invalid | ||
| /// Returns Err(...) on not found or I/O error | ||
| pub fn load_block_header( |
There was a problem hiding this comment.
Can load_block_header just invoke inner_load_block_header after computing the block_path?
…ne if we need a block or microblock stream
This PR backports network fixes and improvements from #2857 that would benefit the Stacks chain. They are:
Adds the
/v2/headersendpoint, which returns a sequence of SIP-003-encoded block headers and consensus hashes (see theExtendedStacksHeaderstruct that this PR adds to represent this data).Adds the
/v2/data_varendpoint, which returns a contract's data variable value and a MARF proof of its existence.Makes the p2p state machine more aggressive at reacting to newly-arrived
BlocksAvailableandMicroblocksAvailablemessages for block and microblock streams that this node does not have. If such messages arrive during an inventory sync, the p2p state machine will immediately transition from the inventory sync work state to the block downloader work state, and immediately proceed to fetch the available block or microblock stream.