Miscellaneous audit fixes #5155

hstove · 2024-09-10T00:49:38Z

This PR implements a few of the smaller items received from an audit. Most of them are pretty trivial (ie improve this logging). Some of them have potential implications for the code's logic, which are worth investigating as a reviewer.

Instead of making a separate PR for each small item, I've bundled them into this single PR. I've added each individual item to a separate commit with (hopefully) a helpful description about what's changed, so I'd recommend going commit-by-commit to review. If there are changes in here that we think need more thought, we can move them into a separate PR.

Added a cfg to warn about unused imports in `net/mod.rs`, which exposed a few unused imports that I've removed. I've also updated other parts of the code that had imports that were only used for testing. In those cases, I've updated the import to only be used in `cfg(test)`.

Previously, because `apply_schema_8` is called after things like `apply_schema_9`, this would override the schema version to be 8. Also removed some trailing whitespace.

I've updated the log line to be a `warn`. More investigation needed on whether we should add explicit validation here to the length of the BitVec.

Previously, there were some cases where we'd `panic`, and some where we'd just handle the error with a log. This updates the logic to always panic.

Previously, we'd only allow timestamps _less than_ 15 seconds away, but our docs state that the timestamp can be _no more than_ 15 seconds away.

In `get_nakamoto_staging_blocks_db_version`, in the case of an error, the function would default to version 1, which could cause an overwrite of tables. This updates the function to return the correct highest version, which I've also moved to a constant.

In `net::relay`, there are many cases where we return an error if unable to announce a new Stacks block, but there was one case where the error was ignored. This updates that case to also return an error.

This adds more verbose and explicit logs when the signer is unable to post a block when handling a `BlockPushed` event.

Previously, two different error cases had the same log message. This updates one of them to have a more description message about when the error occurred.

stacks-signer/src/v0/signer.rs

stackslib/src/chainstate/burn/db/sortdb.rs

stackslib/src/chainstate/nakamoto/miner.rs

stackslib/src/chainstate/nakamoto/staging_blocks.rs

stackslib/src/chainstate/nakamoto/tenure.rs

stackslib/src/net/api/postblock_proposal.rs

stackslib/src/net/relay.rs

Keep it as "max 15 seconds", and update the documentation comments.

hstove · 2024-09-16T18:15:01Z

One of my changes seriously messed something up - pretty much every test that touches sortdb is broken. Needs investigation

stacks-signer/src/v0/signer.rs

stackslib/src/chainstate/burn/db/sortdb.rs

stacks-signer/src/v0/signer.rs

stackslib/src/chainstate/nakamoto/staging_blocks.rs

jcnelson

LGTM; just address Brice's comments

obycode

LGTM!

blockstack-devops · 2024-10-26T00:20:11Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

hstove added 12 commits September 9, 2024 17:30

fix: only update schema_version if lower

2dce84a

Previously, because `apply_schema_8` is called after things like `apply_schema_9`, this would override the schema version to be 8. Also removed some trailing whitespace.

fix: update log when validating pox treatment

20e0a04

I've updated the log line to be a `warn`. More investigation needed on whether we should add explicit validation here to the length of the BitVec.

fix: use consistent error handling when writing to SignerDB

32cd617

Previously, there were some cases where we'd `panic`, and some where we'd just handle the error with a log. This updates the logic to always panic.

fix: typo in NAKAMOTO_TENURES_SCHEMA_1

84c9ed4

Fix: allow block timestamps exactly 15 seconds in the future

48195af

Previously, we'd only allow timestamps _less than_ 15 seconds away, but our docs state that the timestamp can be _no more than_ 15 seconds away.

fix: update processed_time when replacing block

9441849

fix: return error if unable to announce stacks block

a0ad686

In `net::relay`, there are many cases where we return an error if unable to announce a new Stacks block, but there was one case where the error was ignored. This updates that case to also return an error.

fix: log error when failing to push a block

e882ec3

This adds more verbose and explicit logs when the signer is unable to post a block when handling a `BlockPushed` event.

fix: typo in SORTITION_DB_SCHEMA_4

ca22422

fix: more description warning log in NakamotoBlockBuilder

cdf4727

Previously, two different error cases had the same log message. This updates one of them to have a more description message about when the error occurred.

hstove requested review from a team as code owners September 10, 2024 00:49

hstove requested review from jcnelson, obycode and jferrant September 10, 2024 00:49