From 2051b042714c2c0edd7e471e593f54500765590a Mon Sep 17 00:00:00 2001 From: Pavitthra Pandurangan Date: Wed, 10 Mar 2021 15:27:23 -0800 Subject: [PATCH 1/5] docs: update CONTRIBUTING.md with dev workflow info & commit message info; reordered sections --- CONTRIBUTORS.md | 100 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 73 insertions(+), 27 deletions(-) diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 08ebb4f4f..06220c1f2 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -3,14 +3,55 @@ Blockstack Core is open-source software written in Rust. Contributions should adhere to the following best practices. -## Simplicity of implementation +You can find information on joining online community forums (Discord, mailing list etc.) in the [README](README.md). + +#### Table Of Contents + +[Code of Conduct](#code-of-conduct) + +[How Can I Contribute?](#how-can-i-contribute) +* [Development Workflow](#development-workflow) +* [Contributing Conventions](#contributing-conventions) + +[Style](#style) +* [Git Commit Messages](#git-commit-messages) +* [Rust Styleguide](#rust-styleguide) + +[License Agreement](#licensing-and-contributor-license-agreement) + +# Code of Conduct + +This project and everyone participating in it is governed by this [Code of Conduct](CODE_OF_CONDUCT.md). + +# How Can I Contribute? +## Development Workflow + +- For typical development, branch off of the `develop` branch. +- For consensus breaking changes, branch off of the `next` branch. +- For hotfixes, branch off of `master`. + +### Documentation Updates + +- Any major changes should be added to the [CHANGELOG](CHANGELOG.md). +- Mention any required documentation changes in the description of your pull request. +- If adding an RPC endpoint, add an entry for the new endpoint to the OpenAPI spec `./docs/rpc/openapi.yaml`. + +### Each file should include relevant unit tests + +Each Rust file should contain a `mod test {}` definition, in which unit tests +should be supplied for the file's methods. Unit tests should cover a maximal +amount of code paths. + +## Contributing Conventions + +### Simplicity of implementation The most important consideration when accepting or rejecting a contribution is the simplicity (i.e. ease of understanding) of its implementation. Contributions that are "clever" or introduce functionality beyond the scope of the immediate problem they are meant to solve will be rejected. -### Type simplicity +#### Type simplicity Simplicity of implementation includes simplicity of types. Type parameters and associated types should only be used if there are at @@ -19,34 +60,27 @@ least two possible implementations of those types. Lifetime parameters should only be introduced if the compiler cannot deduce them on its own. -## Builds with a stable Rust compiler - +### Builds with a stable Rust compiler We use a recent, stable Rust compiler. Contributions should _not_ require nightly Rust features to build and run. -## Each file should include relevant unit tests - -Each Rust file should contain a `mod test {}` definition, in which unit tests -should be supplied for the file's methods. Unit tests should cover a maximal -amount of code paths. - -## Use built-in logging facilities +### Use built-in logging facilities Blockstack Core implements logging macros in `util::log`. If your code needs to output data, it should use these macros _exclusively_ for doing so. The only exception is code that is explicitly user-facing, such as help documentation. -## Minimal dependencies +### Minimal dependencies Adding new package dependencies is very much discouraged. Exceptions will be granted on a case-by-case basis, and only if deemed absolutely necessary. -## Minimal global macros +### Minimal global macros Adding new global macros is discouraged. Exceptions will only be given if absolutely necessary. -## Minimal compiler warnings +### Minimal compiler warnings Contributions should not trigger compiler warnings if possible, and should not mask compiler warnings with macros. Common sources of compiler warnings that @@ -57,11 +91,31 @@ will not be accepted include, but are not limited to: * variable naming conventions * unhandled return types -## Minimal `unsafe` code +### Minimal `unsafe` code Contributions should not contain `unsafe` blocks if at all possible. -## Code block consistency +### Error definitions + +Each module should include an `Error` enumeration in its `mod.rs` that encodes +errors specific to the module. All error code paths in the module should return +an `Err` type with one of the module's errors. + +# Style +## Git Commit Messages +Aim to use descriptive git commit messages. We try to follow [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/). +The general format is as follows: +``` +[optional scope]: + +[optional body] +[optional footer(s)] +``` +Common types include build, ci, docs, fix, feat, test, refactor, etc. + +## Rust styleguide + +### Code block consistency Surrounding code blocks with `{` and `}` is encouraged, even when the enclosed block is a single statement. Blocks in the same lexical scope must use @@ -91,26 +145,18 @@ match bar { 1..2 => Some("abc"), 3..4 => Some("def"), _ => None -} +} ``` -## Error definitions - -Each module should include an `Error` enumeration in its `mod.rs` that encodes -errors specific to the module. All error code paths in the module should return -an `Err` type with one of the module's errors. - -## Whitespace +### Whitespace All contributions should use the same whitespacing as the rest of the project. Moreover, Pull requests where a large number of changes only deal with whitespace will be rejected. -## Licensing and contributor license agreement. +# Licensing and contributor license agreement Blockstack Core is released under the terms of the GPL version 3. Contributions that are not licensed under compatible terms will be rejected. Moreover, contributions will not be accepted unless _all_ authors accept the project's contributor license agreement. - - From 9979afb028aad83e41e5c4fc9a3fcbeaaeec3f82 Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Thu, 18 Mar 2021 02:51:39 -0400 Subject: [PATCH 2/5] fix #2518 -- advance the microblock download index at a pace independent of what blocks are (or are not) available, so we don't accidentally skip any --- src/net/download.rs | 60 +++++++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/src/net/download.rs b/src/net/download.rs index 71fa1cfb9..c1bebbc74 100644 --- a/src/net/download.rs +++ b/src/net/download.rs @@ -655,9 +655,11 @@ impl BlockDownloader { info!("Remote neighbor {:?} ({:?}) does not have microblock stream indexed at {}", &block_key.neighbor, &block_key.data_url, &block_key.index_block_hash); // the fact that we asked this peer means that it's block inv indicated - // it was present, so the absence is the mark of a broken peer - self.broken_peers.push(event_id); - self.broken_neighbors.push(block_key.neighbor.clone()); + // it was present, so the absence is the mark of a broken peer. + // HOWEVER, there has been some bugs recently about nodes reporting + // invalid microblock streams as present, even though they are + // truly absent. Don't punish these peers with a ban; just don't + // talk to them for a while. } _ => { // wrong message response @@ -1306,7 +1308,11 @@ impl PeerNetwork { )? { Some(hdr) => hdr, None => { - test_debug!("No such block: {:?}", &index_block_hash); + test_debug!( + "{:?}: No such parent block: {:?}", + &self.local_peer, + &index_block_hash + ); continue; } }; @@ -1321,7 +1327,7 @@ impl PeerNetwork { } Err(chainstate_error::DBError(db_error::NotFoundError)) => { // we don't know about this parent block yet - debug!("{:?}: Do not have parent of anchored block {}/{} yet, so cannot ask for the microblocks it produced", &self.local_peer, &consensus_hash, &block_hash); + test_debug!("{:?}: Do not have parent of anchored block {}/{} yet, so cannot ask for the microblocks it produced", &self.local_peer, &consensus_hash, &block_hash); continue; } Err(e) => { @@ -1359,13 +1365,14 @@ impl PeerNetwork { neighbors.clear(); neighbors.append(&mut microblock_stream_neighbors); - test_debug!( - "{:?}: Get microblocks produced by {}/{}, confirmed by {}/{}", + debug!( + "{:?}: Get microblocks produced by {}/{}, confirmed by {}/{}, from up to {} neighbors", &self.local_peer, &parent_consensus_hash, &parent_header.block_hash(), &consensus_hash, - &block_hash + &block_hash, + neighbors.len() ); parent_block_header_opt = Some(parent_header); @@ -1535,6 +1542,8 @@ impl PeerNetwork { } }; + let scan_batch_size = self.burnchain.pox_constants.reward_cycle_length as u64; + if need_blocks { PeerNetwork::with_downloader_state(self, |ref mut network, ref mut downloader| { test_debug!("{:?}: needs blocks", &network.local_peer); @@ -1599,8 +1608,14 @@ impl PeerNetwork { } if max_mblock_height == 0 && next_microblocks_to_try.len() == 0 { - // have no microblocks to try in the first place - max_mblock_height = max_height; + // have no microblocks to try in the first place, so just advance to the + // next batch + debug!( + "No microblocks to try; advance max_mblock_height to {}", + mblock_height + ); + max_mblock_height = mblock_height; + mblock_height += scan_batch_size; } test_debug!("{:?}: at {},{}: {} blocks to get, {} microblock streams to get (up to {},{})", @@ -1619,8 +1634,8 @@ impl PeerNetwork { test_debug!("{:?}: End microblock requests", &network.local_peer); debug!( - "{:?}: create block, microblock requests up to heights ({},{})", - &network.local_peer, &max_height, &max_mblock_height + "{:?}: create block, microblock requests from heights ({},{}) up to heights ({},{}) (so far: {} blocks, {} microblocks queued)", + &network.local_peer, height, mblock_height, max_height, max_mblock_height, downloader.blocks_to_try.len(), downloader.microblocks_to_try.len() ); let now = get_epoch_time_secs(); @@ -1707,6 +1722,7 @@ impl PeerNetwork { "BUG: hashmap both contains and does not contain sortition height", ); if requests.len() == 0 { + debug!("No microblock requests for {}", mblock_height); mblock_height += 1; continue; } @@ -1747,12 +1763,14 @@ impl PeerNetwork { } debug!( - "{:?}: block download scan now at ({},{}) (was ({},{}))", + "{:?}: block download scan now at ({},{}) (was ({},{})), trying {} blocks and {} microblocks", &network.local_peer, height, mblock_height, block_sortition_height, - microblock_sortition_height + microblock_sortition_height, + downloader.blocks_to_try.len(), + downloader.microblocks_to_try.len(), ); if max_height <= next_block_sortition_height @@ -1779,12 +1797,16 @@ impl PeerNetwork { } } - if downloader.blocks_to_try.len() == 0 && downloader.microblocks_to_try.len() == 0 { + if downloader.blocks_to_try.len() == 0 || downloader.microblocks_to_try.len() == 0 { // nothing in this range, so advance sortition range to try for next time - next_block_sortition_height = next_block_sortition_height - + (network.burnchain.pox_constants.reward_cycle_length as u64); - next_microblock_sortition_height = next_microblock_sortition_height - + (network.burnchain.pox_constants.reward_cycle_length as u64); + if downloader.blocks_to_try.len() == 0 { + next_block_sortition_height = next_block_sortition_height + + (network.burnchain.pox_constants.reward_cycle_length as u64); + } + if downloader.microblocks_to_try.len() == 0 { + next_microblock_sortition_height = next_microblock_sortition_height + + (network.burnchain.pox_constants.reward_cycle_length as u64); + } debug!("{:?}: Pessimistically increase block and microblock sortition heights to ({},{})", &network.local_peer, next_block_sortition_height, next_microblock_sortition_height); } From fcf9ffd2c8ab275dfd01730f5f5ef97e71839edd Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Thu, 18 Mar 2021 02:52:19 -0400 Subject: [PATCH 3/5] don't report orphaned microblock streams in block inventory vectors --- src/chainstate/stacks/db/blocks.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/chainstate/stacks/db/blocks.rs b/src/chainstate/stacks/db/blocks.rs index ec51a8e06..2a53b9289 100644 --- a/src/chainstate/stacks/db/blocks.rs +++ b/src/chainstate/stacks/db/blocks.rs @@ -1865,7 +1865,7 @@ impl StacksChainState { ) -> Result { StacksChainState::read_i64s(self.db(), "SELECT staging_microblocks.processed FROM staging_blocks JOIN staging_microblocks ON staging_blocks.parent_anchored_block_hash = staging_microblocks.anchored_block_hash AND staging_blocks.parent_consensus_hash = staging_microblocks.consensus_hash - WHERE staging_blocks.index_block_hash = ?1 AND staging_microblocks.microblock_hash = ?2", &[child_index_block_hash, &parent_microblock_hash]) + WHERE staging_blocks.index_block_hash = ?1 AND staging_microblocks.microblock_hash = ?2 AND staging_microblocks.orphaned = 0", &[child_index_block_hash, &parent_microblock_hash]) .and_then(|processed| { if processed.len() == 0 { Ok(false) From fa1178e33fe391ba35636c0fabdf51429e07f71d Mon Sep 17 00:00:00 2001 From: Jude Nelson Date: Wed, 10 Mar 2021 12:30:01 -0500 Subject: [PATCH 4/5] fix #2491 -- wake up the chains coordinator if a confirmed microblock stream arrives --- src/net/relay.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/net/relay.rs b/src/net/relay.rs index 81827f67b..c9721f2df 100644 --- a/src/net/relay.rs +++ b/src/net/relay.rs @@ -993,11 +993,13 @@ impl Relayer { Relayer::preprocess_pushed_microblocks(network_result, chainstate)?; bad_neighbors.append(&mut new_bad_neighbors); - if new_blocks.len() > 0 || new_microblocks.len() > 0 { + if new_blocks.len() > 0 || new_microblocks.len() > 0 || new_confirmed_microblocks.len() > 0 + { info!( - "Processing newly received Stacks blocks: {}, microblocks: {}", + "Processing newly received Stacks blocks: {}, microblocks: {}, confirmed microblocks: {}", new_blocks.len(), - new_microblocks.len() + new_microblocks.len(), + new_confirmed_microblocks.len() ); if let Some(coord_comms) = coord_comms { if !coord_comms.announce_new_stacks_block() { From f9da01c6208a6871c693f2287af766dde20442fd Mon Sep 17 00:00:00 2001 From: Aaron Blankstein Date: Thu, 18 Mar 2021 09:07:17 -0500 Subject: [PATCH 5/5] add CHANGELOG entry --- CHANGELOG.md | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63a820141..72ea630b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,21 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [2.0.10] + +This is a low-priority hotfix release to address two bugs in the block downloader. The +chainstate directory of 2.0.10 is compatible with 2.0.9. If booting up a node from genesis, or +an existing node has stalled in downloading blocks, this hotfix is necessary for your +node. + +## Fixed + +- Bug in microblocks inventory vector calculation that included invalidated microblocks + as present bit. This bug will impact nodes booting up from genesis, but not affect nodes + currently running at the chain tip (#2518). +- Bug in microblocks downloader logic that would cause the stacks-node to fail to wake-up + to process newly arrived microblocks in certain instances (#2491). + ## [2.0.9] This is a hotfix release for improved handling of arriving Stacks blocks through