Merge pull request #5924 from hstove/feat/rbf-when-config-changed

feat: issue RBF when burnchain/miner config changed

Author: obycode

CHANGELOG.md

@@ -7,8 +7,11 @@ and this project adheres to the versioning scheme outlined in the [README.md](RE
 
 ## [Unreleased]
 
-### Added"
+### Added
+
 - Add fee information to transaction log ending with "success" or "skipped", while building a new block
+- When a miner's config file is updated (ie with a new fee rate), a new block commit is issued using
+  the new values ([#5924](https://github.com/stacks-network/stacks-core/pull/5924))
 
 ### Changed
 

stackslib/src/config/mod.rs

@@ -1197,7 +1197,7 @@ impl std::default::Default for Config {
     }
 }
 
-#[derive(Clone, Debug, Default, Deserialize)]
+#[derive(Clone, Debug, Default, Deserialize, PartialEq)]
 pub struct BurnchainConfig {
     pub chain: String,
     pub mode: String,

testnet/stacks-node/src/globals.rs

@@ -10,7 +10,7 @@ use stacks::chainstate::coordinator::comm::CoordinatorChannels;
 use stacks::chainstate::stacks::db::unconfirmed::UnconfirmedTxMap;
 use stacks::chainstate::stacks::db::StacksChainState;
 use stacks::chainstate::stacks::miner::MinerStatus;
-use stacks::config::MinerConfig;
+use stacks::config::{BurnchainConfig, MinerConfig};
 use stacks::net::NetworkResult;
 use stacks_common::types::chainstate::{BlockHeaderHash, BurnchainHeaderHash, ConsensusHash};
 
@@ -63,6 +63,8 @@ pub struct Globals<T> {
     pub leader_key_registration_state: Arc<Mutex<LeaderKeyRegistrationState>>,
     /// Last miner config loaded
     last_miner_config: Arc<Mutex<Option<MinerConfig>>>,
+    /// Last burnchain config
+    last_burnchain_config: Arc<Mutex<Option<BurnchainConfig>>>,
     /// Last miner spend amount
     last_miner_spend_amount: Arc<Mutex<Option<u64>>>,
     /// burnchain height at which we start mining
@@ -93,6 +95,7 @@ impl<T> Clone for Globals<T> {
             should_keep_running: self.should_keep_running.clone(),
             leader_key_registration_state: self.leader_key_registration_state.clone(),
             last_miner_config: self.last_miner_config.clone(),
+            last_burnchain_config: self.last_burnchain_config.clone(),
             last_miner_spend_amount: self.last_miner_spend_amount.clone(),
             start_mining_height: self.start_mining_height.clone(),
             estimated_winning_probs: self.estimated_winning_probs.clone(),
@@ -125,6 +128,7 @@ impl<T> Globals<T> {
             should_keep_running,
             leader_key_registration_state: Arc::new(Mutex::new(leader_key_registration_state)),
             last_miner_config: Arc::new(Mutex::new(None)),
+            last_burnchain_config: Arc::new(Mutex::new(None)),
             last_miner_spend_amount: Arc::new(Mutex::new(None)),
             start_mining_height: Arc::new(Mutex::new(start_mining_height)),
             estimated_winning_probs: Arc::new(Mutex::new(HashMap::new())),
@@ -355,6 +359,28 @@ impl<T> Globals<T> {
         }
     }
 
+    /// Get the last burnchain config
+    pub fn get_last_burnchain_config(&self) -> Option<BurnchainConfig> {
+        match self.last_burnchain_config.lock() {
+            Ok(last_burnchain_config) => (*last_burnchain_config).clone(),
+            Err(_e) => {
+                error!("FATAL; failed to lock last burnchain config");
+                panic!();
+            }
+        }
+    }
+
+    /// Set the last burnchain config
+    pub fn set_last_burnchain_config(&self, burnchain_config: BurnchainConfig) {
+        match self.last_burnchain_config.lock() {
+            Ok(ref mut last_burnchain_config) => **last_burnchain_config = Some(burnchain_config),
+            Err(_e) => {
+                error!("FATAL; failed to lock last burnchain config");
+                panic!();
+            }
+        }
+    }
+
     /// Get the last miner spend amount
     pub fn get_last_miner_spend_amount(&self) -> Option<u64> {
         match self.last_miner_spend_amount.lock() {

testnet/stacks-node/src/nakamoto_node/relayer.rs

@@ -42,6 +42,7 @@ use stacks::chainstate::stacks::miner::{
     set_mining_spend_amount, signal_mining_blocked, signal_mining_ready,
 };
 use stacks::chainstate::stacks::Error as ChainstateError;
+use stacks::config::BurnchainConfig;
 use stacks::core::mempool::MemPoolDB;
 use stacks::core::STACKS_EPOCH_3_1_MARKER;
 use stacks::monitoring::increment_stx_blocks_mined_counter;
@@ -1101,29 +1102,7 @@ impl RelayerThread {
             return Err(NakamotoNodeError::SnapshotNotFoundForChainTip);
         };
 
-        let burnchain_config = self.config.get_burnchain_config();
-        let last_miner_spend_opt = self.globals.get_last_miner_spend_amount();
-        let force_remine = if let Some(last_miner_spend_amount) = last_miner_spend_opt {
-            last_miner_spend_amount != burnchain_config.burn_fee_cap
-        } else {
-            false
-        };
-        if force_remine {
-            info!(
-                "Miner config changed; updating spend amount {}",
-                burnchain_config.burn_fee_cap
-            );
-        }
-
-        self.globals
-            .set_last_miner_spend_amount(burnchain_config.burn_fee_cap);
-
-        set_mining_spend_amount(
-            self.globals.get_miner_status(),
-            burnchain_config.burn_fee_cap,
-        );
-        // amount of burnchain tokens (e.g. sats) we'll spend across the PoX outputs
-        let burn_fee_cap = burnchain_config.burn_fee_cap;
+        let (_, burnchain_config) = self.check_burnchain_config_changed();
 
         // let's commit, but target the current burnchain tip with our modulus so the commit is
         // only valid if it lands in the targeted burnchain block height
@@ -1155,7 +1134,7 @@ impl RelayerThread {
                 highest_tenure_start_block_header.index_block_hash().0,
             ),
             // the rest of this is the same as epoch2x commits, modulo the new epoch marker
-            burn_fee: burn_fee_cap,
+            burn_fee: burnchain_config.burn_fee_cap,
             apparent_sender: sender,
             key_block_ptr: u32::try_from(key.block_height)
                 .expect("FATAL: burn block height exceeded u32"),
@@ -1703,9 +1682,11 @@ impl RelayerThread {
 
         // update local state
         last_committed.set_txid(&txid);
-        self.globals
-            .counters
-            .bump_naka_submitted_commits(last_committed.burn_tip.block_height, tip_height);
+        self.globals.counters.bump_naka_submitted_commits(
+            last_committed.burn_tip.block_height,
+            tip_height,
+            last_committed.block_commit.burn_fee,
+        );
         self.last_committed = Some(last_committed);
 
         Ok(())
@@ -1768,6 +1749,21 @@ impl RelayerThread {
                "burnchain view changed?" => %burnchain_changed,
                "highest tenure changed?" => %highest_tenure_changed);
 
+        // If the miner spend or config has changed, we want to RBF with new config values.
+        let (burnchain_config_changed, _) = self.check_burnchain_config_changed();
+        let miner_config_changed = self.check_miner_config_changed();
+
+        if burnchain_config_changed || miner_config_changed {
+            info!("Miner spend or config changed; issuing block commit with new values";
+                "miner_spend_changed" => %burnchain_config_changed,
+                "miner_config_changed" => %miner_config_changed,
+            );
+            return Ok(Some(RelayerDirective::IssueBlockCommit(
+                stacks_tip_ch,
+                stacks_tip_bh,
+            )));
+        }
+
         if !burnchain_changed && !highest_tenure_changed {
             // nothing to do
             return Ok(None);
@@ -2136,6 +2132,45 @@ impl RelayerThread {
         debug!("Relayer: handled directive"; "continue_running" => continue_running);
         continue_running
     }
+
+    /// Reload config.burnchain to see if burn_fee_cap has changed.
+    /// If it has, update the miner spend amount and return true.
+    pub fn check_burnchain_config_changed(&self) -> (bool, BurnchainConfig) {
+        let burnchain_config = self.config.get_burnchain_config();
+        let last_burnchain_config_opt = self.globals.get_last_burnchain_config();
+        let burnchain_config_changed =
+            if let Some(last_burnchain_config) = last_burnchain_config_opt {
+                last_burnchain_config != burnchain_config
+            } else {
+                false
+            };
+
+        self.globals
+            .set_last_miner_spend_amount(burnchain_config.burn_fee_cap);
+        self.globals
+            .set_last_burnchain_config(burnchain_config.clone());
+
+        set_mining_spend_amount(
+            self.globals.get_miner_status(),
+            burnchain_config.burn_fee_cap,
+        );
+
+        (burnchain_config_changed, burnchain_config)
+    }
+
+    pub fn check_miner_config_changed(&self) -> bool {
+        let miner_config = self.config.get_miner_config();
+        let last_miner_config_opt = self.globals.get_last_miner_config();
+        let miner_config_changed = if let Some(last_miner_config) = last_miner_config_opt {
+            last_miner_config != miner_config
+        } else {
+            false
+        };
+
+        self.globals.set_last_miner_config(miner_config);
+
+        miner_config_changed
+    }
 }
 
 #[cfg(test)]

testnet/stacks-node/src/run_loop/neon.rs

@@ -117,6 +117,7 @@ pub struct Counters {
     pub naka_signer_pushed_blocks: RunLoopCounter,
     pub naka_miner_directives: RunLoopCounter,
     pub naka_submitted_commit_last_stacks_tip: RunLoopCounter,
+    pub naka_submitted_commit_last_commit_amount: RunLoopCounter,
 
     pub naka_miner_current_rejections: RunLoopCounter,
     pub naka_miner_current_rejections_timeout_secs: RunLoopCounter,
@@ -178,6 +179,7 @@ impl Counters {
         &self,
         committed_burn_height: u64,
         committed_stacks_height: u64,
+        committed_sats_amount: u64,
     ) {
         Counters::inc(&self.naka_submitted_commits);
         Counters::set(
@@ -188,6 +190,10 @@ impl Counters {
             &self.naka_submitted_commit_last_stacks_tip,
             committed_stacks_height,
         );
+        Counters::set(
+            &self.naka_submitted_commit_last_commit_amount,
+            committed_sats_amount,
+        );
     }
 
     pub fn bump_naka_mined_blocks(&self) {

testnet/stacks-node/src/tests/nakamoto_integrations.rs

@@ -11237,3 +11237,145 @@ fn reload_miner_config() {
 
     run_loop_thread.join().unwrap();
 }
+
+/// Test that a new block commit is issued when the miner spend or config changes.
+///
+/// The test boots into Nakamoto. Then, it waits for a block commit on the most recent
+/// tip. The config is updated, and then the test ensures that a new commit was submitted after that
+/// config change.
+#[test]
+#[ignore]
+fn rbf_on_config_change() {
+    if env::var("BITCOIND_TEST") != Ok("1".into()) {
+        return;
+    }
+
+    let (mut conf, _miner_account) = naka_neon_integration_conf(None);
+    let password = "12345".to_string();
+    let _http_origin = format!("http://{}", &conf.node.rpc_bind);
+    conf.connection_options.auth_token = Some(password.clone());
+    conf.miner.wait_on_interim_blocks = Duration::from_secs(1);
+    conf.node.next_initiative_delay = 500;
+    let stacker_sk = setup_stacker(&mut conf);
+    let signer_sk = Secp256k1PrivateKey::random();
+    let signer_addr = tests::to_addr(&signer_sk);
+    let sender_sk = Secp256k1PrivateKey::random();
+    let recipient_sk = Secp256k1PrivateKey::random();
+    let _recipient_addr = tests::to_addr(&recipient_sk);
+    // setup sender + recipient for some test stx transfers
+    // these are necessary for the interim blocks to get mined at all
+    let sender_addr = tests::to_addr(&sender_sk);
+    let old_burn_fee_cap: u64 = 100000;
+    conf.burnchain.burn_fee_cap = old_burn_fee_cap;
+    conf.add_initial_balance(PrincipalData::from(sender_addr).to_string(), 1000000);
+    conf.add_initial_balance(PrincipalData::from(signer_addr).to_string(), 100000);
+
+    test_observer::spawn();
+    test_observer::register(&mut conf, &[EventKeyType::AnyEvent]);
+
+    let mut btcd_controller = BitcoinCoreController::new(conf.clone());
+    btcd_controller
+        .start_bitcoind()
+        .expect("Failed starting bitcoind");
+    let mut btc_regtest_controller = BitcoinRegtestController::new(conf.clone(), None);
+    btc_regtest_controller.bootstrap_chain(201);
+
+    let conf_path =
+        std::env::temp_dir().join(format!("miner-config-test-{}.toml", rand::random::<u64>()));
+    conf.config_path = Some(conf_path.clone().to_str().unwrap().to_string());
+
+    // Make a minimum-viable config file
+    let update_config = |burn_fee_cap: u64, sats_vbyte: u64| {
+        use std::io::Write;
+
+        let new_config = format!(
+            r#"
+            [burnchain]
+            burn_fee_cap = {}
+            satoshis_per_byte = {}
+            "#,
+            burn_fee_cap, sats_vbyte,
+        );
+        // Write to a file
+        let mut file = File::create(&conf_path).unwrap();
+        file.write_all(new_config.as_bytes()).unwrap();
+    };
+
+    let mut run_loop = boot_nakamoto::BootRunLoop::new(conf.clone()).unwrap();
+    let run_loop_stopper = run_loop.get_termination_switch();
+    let counters = run_loop.counters();
+    let Counters {
+        blocks_processed,
+        naka_submitted_commits: commits_submitted,
+        ..
+    } = run_loop.counters();
+
+    let coord_channel = run_loop.coordinator_channels();
+
+    let run_loop_thread = thread::spawn(move || run_loop.start(None, 0));
+    let mut signers = TestSigners::new(vec![signer_sk]);
+    wait_for_runloop(&blocks_processed);
+    boot_to_epoch_3(
+        &conf,
+        &blocks_processed,
+        &[stacker_sk],
+        &[signer_sk],
+        &mut Some(&mut signers),
+        &mut btc_regtest_controller,
+    );
+
+    info!("------------------------- Reached Epoch 3.0 -------------------------");
+
+    blind_signer(&conf, &signers, &counters);
+
+    wait_for_first_naka_block_commit(60, &commits_submitted);
+
+    next_block_and_mine_commit(&mut btc_regtest_controller, 60, &conf, &counters).unwrap();
+
+    let burnchain = conf.get_burnchain();
+    let sortdb = burnchain.open_sortition_db(true).unwrap();
+
+    let tip = SortitionDB::get_canonical_burn_chain_tip(sortdb.conn()).unwrap();
+    let stacks_height = tip.stacks_block_height;
+
+    let mut last_log = Instant::now();
+    last_log -= Duration::from_secs(5);
+    wait_for(30, || {
+        let last_commit = &counters.naka_submitted_commit_last_stacks_tip.get();
+        if last_log.elapsed() >= Duration::from_secs(5) {
+            info!(
+                "---- last_commit: {:?} stacks_height: {:?} ---- ",
+                last_commit, stacks_height
+            );
+            last_log = Instant::now();
+        }
+        Ok(*last_commit >= stacks_height)
+    })
+    .expect("Failed to wait for last commit");
+
+    let commits_before = counters.naka_submitted_commits.get();
+
+    let commit_amount_before = counters.naka_submitted_commit_last_commit_amount.get();
+
+    info!("---- Updating config ----");
+
+    update_config(155000, 57);
+
+    wait_for(30, || {
+        let commit_count = &counters.naka_submitted_commits.get();
+        Ok(*commit_count > commits_before)
+    })
+    .expect("Expected new commit after config change");
+
+    let commit_amount_after = counters.naka_submitted_commit_last_commit_amount.get();
+    assert_eq!(commit_amount_after, 155000);
+    assert_ne!(commit_amount_after, commit_amount_before);
+
+    coord_channel
+        .lock()
+        .expect("Mutex poisoned")
+        .stop_chains_coordinator();
+    run_loop_stopper.store(false, Ordering::SeqCst);
+
+    run_loop_thread.join().unwrap();
+}