-
Notifications
You must be signed in to change notification settings - Fork 0
feat: consider average tenure block fullness for tx fee estimations #2
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
base: develop
Are you sure you want to change the base?
Conversation
// https://github.com/stacks-network/stacks-core/blob/eb865279406d0700474748dc77df100cba6fa98e/stackslib/src/core/mod.rs#L212-L218 | ||
const BLOCK_LIMIT_WRITE_LENGTH = 15_000_000; | ||
const BLOCK_LIMIT_WRITE_COUNT = 15_000; | ||
const BLOCK_LIMIT_READ_LENGTH = 100_000_000; | ||
const BLOCK_LIMIT_READ_COUNT = 15_000; | ||
const BLOCK_LIMIT_RUNTIME = 5_000_000_000; |
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.
Consider not hardcoding these. They could be pulled from either ENV config (less ideal since still duplicates from canonical node data) or perhaps from node directly to keep things in sync. If pulling from node, probably need an interval check for most recent values to update.
@@ -23,6 +23,12 @@ function getReqUrl(req: { url: string; hostname: string }): URL { | |||
|
|||
// https://github.com/stacks-network/stacks-core/blob/20d5137438c7d169ea97dd2b6a4d51b8374a4751/stackslib/src/chainstate/stacks/db/blocks.rs#L338 | |||
const MINIMUM_TX_FEE_RATE_PER_BYTE = 1; |
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.
Where does this value come from? I see the link above, but if there's a way to derive it at runtime vs hard-coded that may be more sustainable for future changes (less to rework).
/// only happens if there is no fee market i.e. if the last N block tenures have not been full. We | ||
/// use a threshold of 90% to determine if a block size dimension is full. | ||
async function shouldUseTransactionMinimumFee(): Promise<boolean> { | ||
const tenureWindow = parseInt(process.env['STACKS_CORE_FEE_TENURE_FULLNESS_WINDOW'] ?? '5'); |
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 always a balance in infinite configuration, but consider not hardcoding the fallback to 5, or if doing so, define it more prominently perhaps at the top of the file or something where constants generally appear. Could get lost in line 141 of functions. This is totally splitting hairs.
const averageCosts = await fastify.db.getTenureAverageExecutionCosts(tenureWindow); | ||
if (!averageCosts) return false; | ||
return ( | ||
averageCosts.read_count < BLOCK_LIMIT_READ_COUNT * 0.9 && |
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.
Consider taking 0.9
literal and make it a variable (const
) at least, don't repeat it so that we don't change it multiple places later and accidentally miss a spot. Could derive from ENV with 0.9 default as with the 5
above.
averageCosts.runtime < BLOCK_LIMIT_RUNTIME * 0.9 | ||
); | ||
} | ||
|
||
const maxBodySize = 10_000_000; // 10 MB max POST body size |
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.
Where is this derived from? Web server or proxy constraints? Could be made configurable and also should be verified as it could cause failure if it exceeds the limits set by whatever web server is running.
reply.statusCode === 200 && | ||
feeEstimationModifier !== null | ||
) { | ||
} else if (getReqUrl(req).pathname === '/v2/fees/transaction' && reply.statusCode === 200) { |
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.
If the API route ever changes, this breaks. Should be configurable or pulled from the Node? routing engine directly if possible.
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.
Case sensitivity may also matter? /V2
vs /v2
etc.
// multiplier. | ||
responseJson.estimations.forEach(estimation => { | ||
// max(min fee, estimate returned by node * configurable modifier) | ||
estimation.fee = Math.max(minFee, Math.round(estimation.fee * modifier)); |
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.
Check: is whole number rounding intended here? I assume so because you probably can't pay fees in fractions.
WHERE type_id = ${DbTxTypeId.TenureChange} | ||
AND canonical = TRUE | ||
AND microblock_canonical = TRUE |
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.
Assuming TenureChange
is an enum or const this is not prone to SQL injection. Ensure an appropriate index exists against txs
table to cover the WHERE query here or else scans will murder this.
WHERE type_id = ${DbTxTypeId.TenureChange} | ||
AND canonical = TRUE | ||
AND microblock_canonical = TRUE | ||
ORDER BY block_height DESC |
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.
Micro-optimization: always try to order in mem instead of making the DB do it, offloads the single point of failure from DB CPU cycles. Probably doesn't matter, this is a nit.
) | ||
SELECT block_height FROM tenure_change_blocks ORDER BY block_height ASC | ||
`; | ||
if (tenureChanges.count != numTenures + 1) return undefined; |
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.
Consider adding comment explaining this logic, as unintuitive to me.
tenureCond = sql`${tenureCond} ELSE ${tenureChanges.length}`; | ||
|
||
// Sum and return each tenure's execution costs. | ||
const result = await sql< |
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.
Consider not using DB as math engine. Pull data and do the SUM
and AVG
in Node to help alleviate DB contention. The tradeoff is if you do this multiple places, you are prone to missing a spot on changes, however that is true of this logic as-is now also.
const currentTenureBlock = tenureChanges[numTenures].block_height; | ||
|
||
// Group blocks by tenure. | ||
let tenureCond = sql``; |
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.
Consider a few lines of comment on algorithm for generating the conditional clause for the SQL.
Modifies the Stacks core fee estimator proxy to follow this algorithm:
All of the previous numbers are configurable via ENV.