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

Enables hot reloading of mining configuration file #3385

Closed
wants to merge 9 commits into from

Conversation

xoloki
Copy link
Collaborator

@xoloki xoloki commented Nov 10, 2022

Enables hot reloading of mining configuration file.

Prior to every mining burnchain transaction, the config file will be reloaded if its modified time is newer and the following settings will be reloaded:

burnchain.burn_fee_cap
burnchain.satoshis_per_byte
burnchain.rbf_fee_increment
burnchain.max_rbf
node.microblock_frequency
node.wait_time_for_microblocks

The following config file items, if missing, will be read from environment variables:

node.seed => STACKS_NODE_SEED
burnchain.username => STACKS_BURNCHAIN_USERNAME
burnchain.password => STACKS_BURNCHAIN_PASSWORD 

Addresses the following issues:

@xoloki xoloki requested a review from igorsyl November 10, 2022 02:33
@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Merging #3385 (29dd595) into develop (a4d4c3b) will decrease coverage by 1.65%.
The diff coverage is 51.20%.

@@             Coverage Diff             @@
##           develop    #3385      +/-   ##
===========================================
- Coverage    32.35%   30.69%   -1.66%     
===========================================
  Files          262      262              
  Lines       212722   212884     +162     
===========================================
- Hits         68816    65348    -3468     
- Misses      143906   147536    +3630     
Impacted Files Coverage Δ
testnet/stacks-node/src/main.rs 0.52% <0.00%> (-0.02%) ⬇️
testnet/stacks-node/src/config.rs 45.41% <31.78%> (-3.57%) ⬇️
testnet/stacks-node/src/run_loop/neon.rs 79.76% <50.00%> (-0.59%) ⬇️
testnet/stacks-node/src/tests/neon_integrations.rs 62.99% <76.00%> (-22.02%) ⬇️
...-node/src/burnchains/bitcoin_regtest_controller.rs 85.47% <97.95%> (+0.24%) ⬆️
testnet/stacks-node/src/neon_node.rs 81.04% <100.00%> (-2.80%) ⬇️
testnet/stacks-node/src/tests/epoch_205.rs 98.03% <100.00%> (+<0.01%) ⬆️
src/cost_estimates/fee_medians.rs 0.00% <0.00%> (-92.38%) ⬇️
src/cost_estimates/fee_rate_fuzzer.rs 0.00% <0.00%> (-60.87%) ⬇️
... and 54 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@xoloki xoloki changed the base branch from master to develop November 10, 2022 04:18
@xoloki xoloki requested a review from donpdonp November 10, 2022 04:21
@igorsyl
Copy link
Contributor

igorsyl commented Nov 10, 2022

Can you run stacks-node with a mock miner configuration to test the config reloading?

@xoloki
Copy link
Collaborator Author

xoloki commented Nov 10, 2022

Can you run stacks-node with a mock miner configuration to test the config reloading?

Sure, do you want me to write an integration test or just run it by hand and check?

@igorsyl
Copy link
Contributor

igorsyl commented Nov 10, 2022

Sure, do you want me to write an integration test or just run it by hand and check?

Integration test would be sweet!


#[derive(Clone, Debug)]
pub struct ConfigLoaderHandle {
handle: std::sync::Arc<std::sync::Mutex<RefCell<ConfigLoader>>>,
Copy link
Member

@jcnelson jcnelson Nov 16, 2022

Choose a reason for hiding this comment

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

You probably don't need (or want) RefCell here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

RefCell removed from ConfigHandle and ConfigLoaderHandle.

@jcnelson
Copy link
Member

On the whole, it's not clear to me why there needs to be a shared ConfigHandle structure. The miner thread could simply load up the config file itself each time it gets spawned, which would obviate the need for any shared state in RAM. It's not like it takes an extraordinary amount of time (especially when compared to mining).

@igorsyl igorsyl removed their request for review February 8, 2023 23:41
@pavitthrap pavitthrap added the frozen PRs that are on hold label Mar 28, 2023
@pavitthrap
Copy link
Contributor

Closing for now, might resume progress in future.

@pavitthrap pavitthrap closed this Mar 28, 2023
@pavitthrap pavitthrap added help wanted Open source contributions actively sought. and removed stacks-labs labels Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frozen PRs that are on hold help wanted Open source contributions actively sought.
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants