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
Feat: reload from config for up-to-date satoshis_per_byte
#4061
Feat: reload from config for up-to-date satoshis_per_byte
#4061
Conversation
b1f82ff
to
17e9864
Compare
I'm not sure if it's desirable to do file reads every time a function needs |
@tippenein can you change the target branch to |
testnet/stacks-node/src/burnchains/bitcoin_regtest_controller.rs
Outdated
Show resolved
Hide resolved
@@ -162,6 +162,13 @@ pub fn make_bitcoin_indexer(config: &Config) -> BitcoinIndexer { | |||
burnchain_indexer | |||
} | |||
|
|||
pub fn get_satoshis_per_byte(config: Config) -> u64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this could be an instance method of Config
, and take &self
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one usage on the LeaderBlockCommitFees
impl that prevented me from doing this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for sending this PR @tippenein! It looks pretty good to me; just had a few of small comments that are easily addressed. EDIT: Also, I agree with @wileyj that this will need to be rebased on develop
, since that's where all new non-consensus-breaking features land (i.e. we follow the gitflow branch model).
Do you think you could add some test coverage? A unit test that verifies that get_satoshis_per_byte()
works as intended would suffice. Thanks!
Given how small the config file is, and how often it'll be loaded, it's probably going to be resident in at least the kernel's block cache in perpetuity. It's not a big deal at all :) |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #4061 +/- ##
=========================================
Coverage 0.04% 0.04%
=========================================
Files 401 401
Lines 287322 286810 -512
=========================================
Hits 136 136
+ Misses 287186 286674 -512 ☔ View full report in Codecov by Sentry. |
d61095d
to
476c74c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM, but can you add a CHANGELOG.md
entry?
85e101c
to
a6a75f2
Compare
Congratulations on your first accepted PR @tippenein! 🥳 |
Congrats, @tippenein :D Welcome :) |
feat #4047
introduces
get_satoshis_per_byte
fn for fetching most up to date value from config