Skip to content
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

Fix: correct burn view for miner block broadcast #5515

Open
wants to merge 55 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
55 commits
Select commit Hold shift + click to select a range
9f3f197
Merge branch 'hotfix/proposal-loads-sortition-view-from-block' into f…
jcnelson Nov 27, 2024
91d0f96
chore: use get_block_burn_view()
jcnelson Nov 27, 2024
b0ebc18
Merge branch 'develop' into fix/burn-view
jcnelson Dec 3, 2024
0d7fbfa
Merge branch 'develop' into fix/burn-view
jcnelson Dec 4, 2024
c9f72e4
chore: add new integration test
jcnelson Dec 5, 2024
8533269
chore: make `MinerReason` debug-printable, and factor out fault injec…
jcnelson Dec 5, 2024
2f16742
fix: consider the possibility that the miner can neither begin a new …
jcnelson Dec 5, 2024
3b81155
chore: track the number of miner directives
jcnelson Dec 5, 2024
08fa52a
chore: integration test to verify that a continue-tenure might not be…
jcnelson Dec 5, 2024
0996fb1
Merge branch 'develop' into fix/burn-view
jcnelson Dec 5, 2024
b110f66
chore: more fixes to differentiate the miner's burn view from the bur…
jcnelson Dec 5, 2024
2d93f24
Merge branch 'fix/burn-view' of https://github.com/stacks-network/sta…
jcnelson Dec 5, 2024
9b53d70
chore: more checks on burn view changes
jcnelson Dec 6, 2024
c8974b2
Merge branch 'feat/time-based-tenure-extend' into fix/burn-view
jcnelson Dec 7, 2024
8be6f90
Merge branch 'feat/time-based-tenure-extend' into fix/burn-view
jcnelson Dec 9, 2024
22a9815
Merge branch 'develop' into fix/burn-view
jcnelson Dec 9, 2024
4c9155b
Cargo fmt
jferrant Dec 10, 2024
eb62628
chore: record last sortition
jcnelson Dec 15, 2024
93cf523
chore: remove EmptyTenure miner reason, since it shouldn't ever be used
jcnelson Dec 15, 2024
a7a0b19
chore: factor logic for checking for a tenure-extend into a single fu…
jcnelson Dec 15, 2024
a2f010e
chore; drop needs_initial_block
jcnelson Dec 15, 2024
48e7468
test: finish check that the hotfix ensures that the correct burn view…
jcnelson Dec 15, 2024
f488b35
chore: delete old code
jcnelson Dec 15, 2024
73821ad
Merge branch 'fix/burn-view' of https://github.com/stacks-network/sta…
jcnelson Dec 15, 2024
818c064
Merge branch 'develop' into fix/burn-view
jcnelson Dec 16, 2024
06e2764
chore: clean up compile error and warnings
jcnelson Dec 16, 2024
fd28d81
Merge branch 'fix/burn-view' of https://github.com/stacks-network/sta…
jcnelson Dec 16, 2024
7abaaca
feat: tenure_extend_wait_secs: a config option to wait for a block-fo…
jcnelson Dec 20, 2024
06096ee
fix: allow a BlockFound to be produced if the Relayer determines that…
jcnelson Dec 20, 2024
7631f41
chore: fix choose_miner_directive() to attempt to continue a tenure i…
jcnelson Dec 20, 2024
0f7ada4
chore: fix tests
jcnelson Dec 20, 2024
6b18429
chore: expect a TenureExtend for a flash block
jcnelson Dec 20, 2024
1819f2a
Merge branch 'develop' into fix/burn-view
jcnelson Dec 20, 2024
f146ade
Merge branch 'develop' into fix/burn-view
jcnelson Jan 4, 2025
70833cd
refactor: use `TestFlag` for more flags
obycode Jan 9, 2025
6fe5d2d
fix: pause Stacks mining while mining blocks for miner eligibility
obycode Jan 9, 2025
911560c
test: add wait to ensure tip has advanced
obycode Jan 9, 2025
1c31090
test: add new test for tenure extend
obycode Jan 9, 2025
24193f8
Merge branch 'develop' into fix/burn-view
jcnelson Jan 13, 2025
9de3f84
fix: `won_sortition` calculation in relayer
obycode Jan 13, 2025
5590ec0
chore: get tenure_extend_after_failed_miner to pass
jcnelson Jan 13, 2025
45028e4
Merge branch 'fix/burn-view' of https://github.com/stacks-network/sta…
jcnelson Jan 13, 2025
27519c3
chore: expand test_tenure_extend_from_flashblocks to check that all b…
jcnelson Jan 13, 2025
17d6edc
fix: build issue; fix relayer to always start a new tenure if the cur…
jcnelson Jan 13, 2025
99d3eff
test: change VRF proof calculation to test a comment from @obycode
jcnelson Jan 13, 2025
49d5d65
Merge branch 'develop' into fix/burn-view
jcnelson Jan 14, 2025
262ee7d
chore: revert to LazyStatic
jcnelson Jan 14, 2025
792fcfc
Merge branch 'develop' into fix/burn-view
jcnelson Jan 14, 2025
62c9f13
chore: add docstrings, and (to test) disable the check_burn_view_chan…
jcnelson Jan 15, 2025
cc5e2fd
Merge branch 'develop' into fix/burn-view
jcnelson Jan 15, 2025
618c3a0
chore: cargo fmt
jcnelson Jan 15, 2025
fa823b1
test: disable check_burn_view_changed()
jcnelson Jan 15, 2025
a6a42cf
Merge branch 'develop' into fix/burn-view
jcnelson Jan 15, 2025
8e9303a
fix: remove compile warnings that prevent CI from running
jcnelson Jan 15, 2025
5c2cc18
Merge branch 'develop' into fix/burn-view
jcnelson Jan 15, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .github/workflows/bitcoin-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ jobs:
- tests::signer::v0::continue_after_tenure_extend
- tests::signer::v0::tenure_extend_after_idle_signers
- tests::signer::v0::tenure_extend_after_idle_miner
- tests::signer::v0::tenure_extend_after_failed_miner
- tests::signer::v0::tenure_extend_succeeds_after_rejected_attempt
- tests::signer::v0::stx_transfers_dont_effect_idle_timeout
- tests::signer::v0::idle_tenure_extend_active_mining
Expand Down Expand Up @@ -158,6 +159,7 @@ jobs:
- tests::nakamoto_integrations::sip029_coinbase_change
- tests::nakamoto_integrations::clarity_cost_spend_down
- tests::nakamoto_integrations::v3_blockbyheight_api_endpoint
- tests::nakamoto_integrations::test_tenure_extend_from_flashblocks
# TODO: enable these once v1 signer is supported by a new nakamoto epoch
# - tests::signer::v1::dkg
# - tests::signer::v1::sign_request_rejected
Expand Down
3 changes: 2 additions & 1 deletion stacks-signer/src/chainstate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ impl SortitionsView {
return Ok(false);
}
}
ProposedBy::LastSortition(_last_sortition) => {
ProposedBy::LastSortition(last_sortition) => {
// should only consider blocks from the last sortition if the new sortition was invalidated
// before we signed their first block.
if self.cur_sortition.miner_status
Expand All @@ -333,6 +333,7 @@ impl SortitionsView {
"proposed_block_consensus_hash" => %block.header.consensus_hash,
"proposed_block_signer_sighash" => %block.header.signer_signature_hash(),
"current_sortition_miner_status" => ?self.cur_sortition.miner_status,
"last_sortition" => %last_sortition.consensus_hash
);
return Ok(false);
}
Expand Down
7 changes: 7 additions & 0 deletions stackslib/src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ const DEFAULT_FIRST_REJECTION_PAUSE_MS: u64 = 5_000;
const DEFAULT_SUBSEQUENT_REJECTION_PAUSE_MS: u64 = 10_000;
const DEFAULT_BLOCK_COMMIT_DELAY_MS: u64 = 20_000;
const DEFAULT_TENURE_COST_LIMIT_PER_BLOCK_PERCENTAGE: u8 = 25;
const DEFAULT_TENURE_EXTEND_WAIT_SECS: u64 = 30;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems pretty aggressive to me. I think what this means is that miners will attempt to extend even if the next sortition has a valid winner after 30 seconds. What percentage of tenures mine their first block within this time period (and have the block processed by a follower node)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relevant value in the signer set defaults to 600 seconds: https://github.com/stacks-network/stacks-core/blob/master/stacks-signer/src/config.rs#L37

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, sorry for the confusion earlier. I was thinking first_proposal_burn_block_timing_secs was the relevant config here, which defaults to 60s, but you're right.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what this means is that miners will attempt to extend even if the next sortition has a valid winner after 30 seconds.

I'm happy to change the timeout, but it doesn't prevent dueling miners from arising. It's possible that in $TIMEOUT + 1 seconds, the winning miner comes online and signers reject it.

We all good with 600 seconds?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference would be to remove this behavior from this PR, and then we should start an issue describing this behavior, and we can go from there. I have concerns about he particular timeout chosen, but maybe other concerns as well, and I think its worth separating this behavior from the fix that this PR is trying to address.

Copy link
Member Author

@jcnelson jcnelson Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, which behavior do you want to remove from the PR?


// This should be greater than the signers' timeout. This is used for issuing fallback tenure extends
const DEFAULT_TENURE_TIMEOUT_SECS: u64 = 420;

Expand Down Expand Up @@ -2149,6 +2151,8 @@ pub struct MinerConfig {
pub block_commit_delay: Duration,
/// The percentage of the remaining tenure cost limit to consume each block.
pub tenure_cost_limit_per_block_percentage: Option<u8>,
/// The number of seconds to wait to try to continue a tenure if a BlockFound is expected
pub tenure_extend_wait_secs: Duration,
/// Duration to wait before attempting to issue a tenure extend
pub tenure_timeout: Duration,
}
Expand Down Expand Up @@ -2187,6 +2191,7 @@ impl Default for MinerConfig {
tenure_cost_limit_per_block_percentage: Some(
DEFAULT_TENURE_COST_LIMIT_PER_BLOCK_PERCENTAGE,
),
tenure_extend_wait_secs: Duration::from_secs(DEFAULT_TENURE_EXTEND_WAIT_SECS),
tenure_timeout: Duration::from_secs(DEFAULT_TENURE_TIMEOUT_SECS),
}
}
Expand Down Expand Up @@ -2582,6 +2587,7 @@ pub struct MinerConfigFile {
pub subsequent_rejection_pause_ms: Option<u64>,
pub block_commit_delay_ms: Option<u64>,
pub tenure_cost_limit_per_block_percentage: Option<u8>,
pub tenure_extend_wait_secs: Option<u64>,
pub tenure_timeout_secs: Option<u64>,
}

Expand Down Expand Up @@ -2723,6 +2729,7 @@ impl MinerConfigFile {
subsequent_rejection_pause_ms: self.subsequent_rejection_pause_ms.unwrap_or(miner_default_config.subsequent_rejection_pause_ms),
block_commit_delay: self.block_commit_delay_ms.map(Duration::from_millis).unwrap_or(miner_default_config.block_commit_delay),
tenure_cost_limit_per_block_percentage,
tenure_extend_wait_secs: self.tenure_extend_wait_secs.map(Duration::from_secs).unwrap_or(miner_default_config.tenure_extend_wait_secs),
tenure_timeout: self.tenure_timeout_secs.map(Duration::from_secs).unwrap_or(miner_default_config.tenure_timeout),
})
}
Expand Down
17 changes: 11 additions & 6 deletions stackslib/src/net/api/postblock_proposal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
// along with this program. If not, see <http://www.gnu.org/licenses/>.

use std::io::{Read, Write};
#[cfg(any(test, feature = "testing"))]
use std::sync::LazyLock;
use std::thread::{self, JoinHandle, Thread};
#[cfg(any(test, feature = "testing"))]
use std::time::Duration;
Expand All @@ -35,6 +37,8 @@ use stacks_common::types::net::PeerHost;
use stacks_common::types::StacksPublicKeyBuffer;
use stacks_common::util::hash::{hex_bytes, to_hex, Hash160, Sha256Sum, Sha512Trunc256Sum};
use stacks_common::util::retry::BoundReader;
#[cfg(any(test, feature = "testing"))]
use stacks_common::util::tests::TestFlag;
use stacks_common::util::{get_epoch_time_ms, get_epoch_time_secs};

use crate::burnchains::affirmation::AffirmationMap;
Expand Down Expand Up @@ -67,11 +71,11 @@ use crate::net::{
use crate::util_lib::db::Error as DBError;

#[cfg(any(test, feature = "testing"))]
pub static TEST_VALIDATE_STALL: std::sync::Mutex<Option<bool>> = std::sync::Mutex::new(None);
pub static TEST_VALIDATE_STALL: LazyLock<TestFlag<bool>> = LazyLock::new(TestFlag::default);
#[cfg(any(test, feature = "testing"))]
/// Artificial delay to add to block validation.
pub static TEST_VALIDATE_DELAY_DURATION_SECS: std::sync::Mutex<Option<u64>> =
std::sync::Mutex::new(None);
pub static TEST_VALIDATE_DELAY_DURATION_SECS: LazyLock<TestFlag<u64>> =
LazyLock::new(TestFlag::default);

// This enum is used to supply a `reason_code` for validation
// rejection responses. This is serialized as an enum with string
Expand Down Expand Up @@ -353,10 +357,10 @@ impl NakamotoBlockProposal {
) -> Result<BlockValidateOk, BlockValidateRejectReason> {
#[cfg(any(test, feature = "testing"))]
{
if *TEST_VALIDATE_STALL.lock().unwrap() == Some(true) {
if TEST_VALIDATE_STALL.get() {
// Do an extra check just so we don't log EVERY time.
warn!("Block validation is stalled due to testing directive.");
while *TEST_VALIDATE_STALL.lock().unwrap() == Some(true) {
while TEST_VALIDATE_STALL.get() {
std::thread::sleep(std::time::Duration::from_millis(10));
}
info!(
Expand All @@ -368,7 +372,8 @@ impl NakamotoBlockProposal {

#[cfg(any(test, feature = "testing"))]
{
if let Some(delay) = *TEST_VALIDATE_DELAY_DURATION_SECS.lock().unwrap() {
let delay = TEST_VALIDATE_DELAY_DURATION_SECS.get();
if delay > 0 {
warn!("Sleeping for {} seconds to simulate slow processing", delay);
thread::sleep(Duration::from_secs(delay));
}
Expand Down
Loading
Loading