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

min_bucket_feerate 1e5 to 1000 #188

Merged
merged 1 commit into from Mar 10, 2024
Merged

min_bucket_feerate 1e5 to 1000 #188

merged 1 commit into from Mar 10, 2024

Conversation

ghost
Copy link

@ghost ghost commented Mar 8, 2024

Minimum and Maximum values for tracking feerates The min_bucket_feerate should just be set to the lowest reasonable feerate we might ever want to track.

Historically this has been 1000 since it was inheriting default_min_relay_tx_fee
changing it is disruptive as it invalidates old estimates files. I think we should change it back to 1e3.

static constexpr double MIN_BUCKET_FEERATE = 1e5;
back to 1e3
static constexpr double MIN_BUCKET_FEERATE = 1000;

@JaredTate
Copy link

So this is just reverting to where it was two weeks ago? Was there a specific test you found this was affecting? Looks like MIN_BUCKET_FEERATE is only used in one place:

CBlockPolicyEstimator::CBlockPolicyEstimator()
: nBestSeenHeight(0), firstRecordedHeight(0), historicalFirst(0), historicalBest(0), trackedTxs(0), untrackedTxs(0)
{
static_assert(MIN_BUCKET_FEERATE > 0, "Min feerate must be nonzero");
size_t bucketIndex = 0;
for (double bucketBoundary = MIN_BUCKET_FEERATE; bucketBoundary <= MAX_BUCKET_FEERATE; bucketBoundary *= FEE_SPACING, bucketIndex++) {
buckets.push_back(bucketBoundary);
bucketMap[bucketBoundary] = bucketIndex;
}
buckets.push_back(INF_FEERATE);
bucketMap[INF_FEERATE] = bucketIndex;
assert(bucketMap.size() == buckets.size());
feeStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, MED_BLOCK_PERIODS, MED_DECAY, MED_SCALE));
shortStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, SHORT_BLOCK_PERIODS, SHORT_DECAY, SHORT_SCALE));
longStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, LONG_BLOCK_PERIODS, LONG_DECAY, LONG_SCALE));
// If the fee estimation file is present, read recorded estimations
fs::path est_filepath = gArgs.GetDataDirNet() / FEE_ESTIMATES_FILENAME;
CAutoFile est_file(fsbridge::fopen(est_filepath, "rb"), SER_DISK, CLIENT_VERSION);
if (est_file.IsNull() || !Read(est_file)) {
LogPrintf("Failed to read fee estimates from %s. Continue anyway.\n", est_filepath.string());
}
}

@ghost
Copy link
Author

ghost commented Mar 8, 2024

So this is just reverting to where it was two weeks ago? Was there a specific test you found this was affecting? Looks like MIN_BUCKET_FEERATE is only used in one place:

CBlockPolicyEstimator::CBlockPolicyEstimator()
: nBestSeenHeight(0), firstRecordedHeight(0), historicalFirst(0), historicalBest(0), trackedTxs(0), untrackedTxs(0)
{
static_assert(MIN_BUCKET_FEERATE > 0, "Min feerate must be nonzero");
size_t bucketIndex = 0;
for (double bucketBoundary = MIN_BUCKET_FEERATE; bucketBoundary <= MAX_BUCKET_FEERATE; bucketBoundary *= FEE_SPACING, bucketIndex++) {
buckets.push_back(bucketBoundary);
bucketMap[bucketBoundary] = bucketIndex;
}
buckets.push_back(INF_FEERATE);
bucketMap[INF_FEERATE] = bucketIndex;
assert(bucketMap.size() == buckets.size());
feeStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, MED_BLOCK_PERIODS, MED_DECAY, MED_SCALE));
shortStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, SHORT_BLOCK_PERIODS, SHORT_DECAY, SHORT_SCALE));
longStats = std::unique_ptr<TxConfirmStats>(new TxConfirmStats(buckets, bucketMap, LONG_BLOCK_PERIODS, LONG_DECAY, LONG_SCALE));
// If the fee estimation file is present, read recorded estimations
fs::path est_filepath = gArgs.GetDataDirNet() / FEE_ESTIMATES_FILENAME;
CAutoFile est_file(fsbridge::fopen(est_filepath, "rb"), SER_DISK, CLIENT_VERSION);
if (est_file.IsNull() || !Read(est_file)) {
LogPrintf("Failed to read fee estimates from %s. Continue anyway.\n", est_filepath.string());
}
}

Not find a test that is effected by this yet. Maybe there is none. But i think that the max_bucket_feerate 1e10 is good and the min_bucket_feerate should be 1e3

Copy link

@JaredTate JaredTate left a comment

Choose a reason for hiding this comment

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

ACK. Sounds good.

Copy link
Member

@ycagel ycagel left a comment

Choose a reason for hiding this comment

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

cACK. Thank you @Jongjan88.

Copy link
Member

@gto90 gto90 left a comment

Choose a reason for hiding this comment

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

utACK

@gto90 gto90 merged commit 84150a0 into DigiByte-Core:develop Mar 10, 2024
@ghost ghost deleted the minipr branch March 10, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants