Skip to content

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 38 additions & 10 deletions src/api/routes/core-node-rpc-proxy.ts
Original file line number Diff line number Diff line change
@@ -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;

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).

// 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;
Comment on lines +26 to +31

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.


interface FeeEstimation {
fee: number;
@@ -128,6 +134,22 @@ export const CoreNodeRpcProxyRouter: FastifyPluginAsync<
}
}

/// Checks if we should modify all transaction fee estimations to always use the minimum fee. This
/// 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');

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 &&

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.read_length < BLOCK_LIMIT_READ_LENGTH * 0.9 &&
averageCosts.write_count < BLOCK_LIMIT_WRITE_COUNT * 0.9 &&
averageCosts.write_length < BLOCK_LIMIT_WRITE_LENGTH * 0.9 &&
averageCosts.runtime < BLOCK_LIMIT_RUNTIME * 0.9
);
}

const maxBodySize = 10_000_000; // 10 MB max POST body size

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.

fastify.addContentTypeParser(
'application/octet-stream',
@@ -233,11 +255,7 @@ export const CoreNodeRpcProxyRouter: FastifyPluginAsync<
const txId = responseBuffer.toString();
await logTxBroadcast(txId);
await reply.send(responseBuffer);
} else if (
getReqUrl(req).pathname === '/v2/fees/transaction' &&
reply.statusCode === 200 &&
feeEstimationModifier !== null
) {
} else if (getReqUrl(req).pathname === '/v2/fees/transaction' && reply.statusCode === 200) {

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.

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.

const reqBody = req.body as {
estimated_len?: number;
transaction_payload: string;
@@ -248,13 +266,23 @@ export const CoreNodeRpcProxyRouter: FastifyPluginAsync<
reqBody.transaction_payload.length / 2
);
const minFee = txSize * MINIMUM_TX_FEE_RATE_PER_BYTE;
const modifier = feeEstimationModifier;
const modifier = feeEstimationModifier ?? 1.0;
const responseBuffer = await readRequestBody(response as ServerResponse);
const responseJson = JSON.parse(responseBuffer.toString()) as FeeEstimateResponse;
responseJson.estimations.forEach(estimation => {
// max(min fee, estimate returned by node * configurable modifier)
estimation.fee = Math.max(minFee, Math.round(estimation.fee * modifier));
});

if (await shouldUseTransactionMinimumFee()) {
// Tenures are not full i.e. there's no fee market. Return the minimum fee.
responseJson.estimations.forEach(estimation => {
estimation.fee = minFee;
});
} else {
// Fall back to Stacks core's estimate, but modify it according to the ENV configured
// multiplier.
responseJson.estimations.forEach(estimation => {
// max(min fee, estimate returned by node * configurable modifier)
estimation.fee = Math.max(minFee, Math.round(estimation.fee * modifier));

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.

});
}
await reply.removeHeader('content-length').send(JSON.stringify(responseJson));
} else {
await reply.send(response);
79 changes: 79 additions & 0 deletions src/datastore/pg-store.ts
Original file line number Diff line number Diff line change
@@ -4533,4 +4533,83 @@ export class PgStore extends BasePgStore {
`;
return parseInt(result[0]?.count ?? '0');
}

async getTenureAverageExecutionCosts(numTenures: number) {
return await this.sqlTransaction(async sql => {
// Get the last N+1 tenure change blocks so we can get a total of N tenures.
const tenureChanges = await sql<{ block_height: number }[]>`
WITH tenure_change_blocks AS (
SELECT block_height
FROM txs
WHERE type_id = ${DbTxTypeId.TenureChange}
AND canonical = TRUE
AND microblock_canonical = TRUE
Comment on lines +4544 to +4546

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.

ORDER BY block_height DESC

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.

LIMIT ${numTenures + 1}
)
SELECT block_height FROM tenure_change_blocks ORDER BY block_height ASC
`;
if (tenureChanges.count != numTenures + 1) return undefined;

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.

const firstTenureBlock = tenureChanges[0].block_height;
const currentTenureBlock = tenureChanges[numTenures].block_height;

// Group blocks by tenure.
let tenureCond = sql``;

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.

let low = firstTenureBlock;
let high = low;
for (let i = 1; i < tenureChanges.length; i++) {
high = tenureChanges[i].block_height;
tenureCond = sql`${tenureCond} WHEN block_height BETWEEN ${low} AND ${high} THEN ${i}`;
low = high + 1;
}
tenureCond = sql`${tenureCond} ELSE ${tenureChanges.length}`;

// Sum and return each tenure's execution costs.
const result = await sql<

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.

{
runtime: number;
read_count: number;
read_length: number;
write_count: number;
write_length: number;
}[]
>`
WITH grouped_blocks AS (
SELECT
block_height,
execution_cost_runtime,
execution_cost_read_count,
execution_cost_read_length,
execution_cost_write_count,
execution_cost_write_length,
CASE
${tenureCond}
END AS tenure_index
FROM blocks
WHERE block_height >= ${firstTenureBlock}
AND block_height < ${currentTenureBlock}
ORDER BY block_height DESC
),
tenure_costs AS (
SELECT
SUM(execution_cost_runtime) AS runtime,
SUM(execution_cost_read_count) AS read_count,
SUM(execution_cost_read_length) AS read_length,
SUM(execution_cost_write_count) AS write_count,
SUM(execution_cost_write_length) AS write_length
FROM grouped_blocks
GROUP BY tenure_index
ORDER BY tenure_index DESC
)
SELECT
AVG(runtime) AS runtime,
AVG(read_count) AS read_count,
AVG(read_length) AS read_length,
AVG(write_count) AS write_count,
AVG(write_length) AS write_length
FROM tenure_costs
`;
return result[0];
});
}
}