-
Notifications
You must be signed in to change notification settings - Fork 18
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
3D pricing metrics #107
base: main
Are you sure you want to change the base?
3D pricing metrics #107
Conversation
Signed-off-by: Fabio Di Fabio <[email protected]>
- Introduce block listener in LineaTransactionSelectorPlugin to handle transaction profitability - Integrate transaction selector handler for processing transactions from the latest block - Extract profitability calculation into a separate method for clarity and maintainability Signed-off-by: Ade Lucas <[email protected]>
Signed-off-by: Ade Lucas <[email protected]>
Signed-off-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Fabio Di Fabio <[email protected]>
…orPlugin - Implement: Node's perception on Profitability levels of TxPool's contents (lo, hi, avg of TransactionProfitabilityCalculator.profitablePriorityFeePerGas(transaction) / transaction.priorityFeePerGas). Signed-off-by: Ade Lucas <[email protected]>
…orPlugin - Implement: Node's perception on Profitability levels of TxPool's contents (lo, hi, avg of TransactionProfitabilityCalculator.profitablePriorityFeePerGas(transaction) / transaction.priorityFeePerGas). Signed-off-by: Ade Lucas <[email protected]>
Signed-off-by: Ade Lucas <[email protected]>
Signed-off-by: Fabio Di Fabio <[email protected]>
Signed-off-by: Fabio Di Fabio <[email protected]>
…ics work Signed-off-by: Ade Lucas <[email protected]>
Signed-off-by: Ade Lucas <[email protected]>
…SelectorTest.java Signed-off-by: Ade Lucas <[email protected]>
Signed-off-by: Ade Lucas <[email protected]>
… enabled Signed-off-by: Fabio Di Fabio <[email protected]>
644c543
to
9ac2555
Compare
Signed-off-by: Fabio Di Fabio <[email protected]>
9ac2555
to
fcb4001
Compare
Signed-off-by: Fabio Di Fabio <[email protected]>
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.
Non-blockion suggestions. LGTM
hidden = true, | ||
paramLabel = "<FLOAT[]>", | ||
description = | ||
"List of buckets to use to create the histogram for profitability metrics (default: ${DEFAULT-VALUE})") |
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 description could be more informative. They are buckets of what? profit for the sequencer in wei, gwei, or ? is it the tx fee revenue, or the net profit between posting to L1 vs fee on linea, etc.
I can find it in the code, but I think the option description could clear it up
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.
updated, to make it clearer
evaluationContext.getTransactionGasPrice().subtract(baseFee); | ||
final var ratio = | ||
effectivePriorityFee.getValue().doubleValue() | ||
/ profitablePriorityFeePerGas.getValue().doubleValue(); |
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.
are we guaranteed that profitablePriorityFeePerGas is never zero.
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.
it could be zero only the pricing conf for variable and fixed cost is zero, that seems unreasonable, and according to the Java rules that operation will result in Double.POSITIVE_INFINITY
Signed-off-by: Fabio Di Fabio <[email protected]>
db537b5
to
9f2708e
Compare
Signed-off-by: Fabio Di Fabio <[email protected]>
sequencer/src/main/java/net/consensys/linea/metrics/HistogramMetrics.java
Show resolved
Hide resolved
histogram.labels(labelValues).observe(value); | ||
} | ||
|
||
public void setMinMax(final double min, final double max, final String... labelValues) { |
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.
Why do we need it as a separate method? Do you think that O(N) additional comparisons in track
will impact the performance?
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.
I will review if it is possible to perform all the updates in a single method
@@ -113,6 +121,45 @@ public void start() { | |||
l1L2BridgeSharedConfiguration(), | |||
rejectedTxJsonRpcManager)); | |||
|
|||
if (metricCategoryRegistry.isMetricCategoryEnabled(TX_POOL_PROFITABILITY)) { |
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.
Similarly to how it's done for ProfitableTransactionSelector
maybe instead of evaluating each transaction from txpool directly, maybe it would be more efficient and less invasive if we capture txpool profitability metrics in ProfitabilityValidator
?
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.
we thought about that, but since a tx could stay in the pool for a long time, the value calculated at ProfitabilityValidator
time could be different from the current value, due to the changes to the market conditions and the extra data pricing conf, so basically it will be a different metric, that represents the values at validation time, instead of the current status in the txpool.
If you think that collecting metrics in the ProfitabilityValidator
is good enough, then yes it will be more efficient.
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.
I see what you mean. Let's leave it the way you did and if it causes any performance issues, we can always disable it
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.
since the txpool should have a similar content on every node, it is possible to enable this metric only on dedicated or not critical nodes.
sequencer/src/main/java/net/consensys/linea/config/LineaProfitabilityCliOptions.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Fabio Di Fabio <[email protected]>
This PR exposes the following metrics
1 Node's perception on the latest extraData contents (variable cost, fixed cost, eth gas price) as gauges under the PRICING_CONF category
1 Node's perception on Profitability levels of TxPool's contents (lo, hi, avg of transaction.priorityFeePerGas / TransactionProfitabilityCalculator.profitablePriorityFeePerGas(transaction)) as histogram under the TX_POOL_PROFITABILITY categoty
1 Same metrics as above, but in context of last sealed block (this is relevant only for Sequencer) as histogram under the SEQUENCER_PROFITABILITY category
Each metric category can be enabled indipendently, using the standard metrics-category option.
For histograms, it is possible to configure the buckets using the new conf option
plugin-linea-profitability-metrics-buckets
, that by default is set to0.9, 1.0, 1.2, 2, 5, 10, 100, 1000
names of the new exported metrics
fixes https://github.com/Consensys/protocol-misc/issues/953