[wip] feat: implement quicknode geth vs reth output comparison #775
Conversation
jsy1218
changed the title
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
graphite-app
bot
requested review from
cgkol,
mikeki,
a team,
xrsv and
uni-guillaume
Graphite Automations"Request reviewers once CI passes on routing-api repo" took an action on this PR • (07/02/24)6 reviewers were added and 1 assignee was added to this PR based on 's automation. |
| logRpcResponseMatch(method: string, otherProvider: SingleJsonRpcProvider) { | ||
| metric.putMetric( | ||
| `${this.metricPrefix}_other_provider_${otherProvider.providerId}_method_${method}_rpc_match`, | ||
| 1, | ||
| MetricLoggerUnit.Count | ||
| ) | ||
| } | ||
|
|
||
| logRpcResponseMismatch(method: string, otherProvider: SingleJsonRpcProvider) { | ||
| metric.putMetric( | ||
| `${this.metricPrefix}_other_provider_${otherProvider.providerId}_method_${method}_rpc_mismatch`, | ||
| 1, | ||
| MetricLoggerUnit.Count | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
should these 2 be in UniJsonRpcProvider? it kind of feels like SingleJsonRpcProvider doesn't need to know about these methods but I could be convinced otherwise
There was a problem hiding this comment.
Eventually all those stuffs should be migrated to the standalone RPC service. I'm not too worried about the small gymnastics like this.
lib/rpc/UniJsonRpcProvider.ts
Outdated
| @@ -230,7 +231,20 @@ export class UniJsonRpcProvider extends StaticJsonRpcProvider { | |||
| // Within each provider latency shadow evaluation, we should do block I/O, | |||
| // because NodeJS runs in single thread, so it's important to make sure | |||
| // we benchmark the latencies correctly based on the single-threaded sequential evaluation. | |||
| await provider.evaluateLatency(methodName, args) | |||
| const evaluatedProviderResponse = await (provider as any)[`evaluateLatency`](methodName, ...args) | |||
There was a problem hiding this comment.
why do we need to cast to any?
There was a problem hiding this comment.
actually we don't need to. I was copy paste
routing-api/lib/rpc/UniJsonRpcProvider.ts
Line 294 in 5bf6608
lib/rpc/UniJsonRpcProvider.ts
Outdated
| // 0.5 | ||
| // ] | ||
| // } | ||
| const serializedProviderResponse = JSON.stringify(providerResponse) |
There was a problem hiding this comment.
@jsy1218, apologies for the drive-by comment, but one thing to be careful of here is that JSON.stringify does not preserve the order for keys in an object (it does, however, preserve array ordering). You may want to consider a deeper comparison as there's a chance this leads to a high number of false negatives.
|
@jsy1218 just curious what are the benefits switching to RETH? is it supposed to be faster? |
@xrsv Short-term, we benefit from a faster quote (presumably RETH output has the same correctness as GETH). Long-term, the entire ethereum ecosystem benefit from a higher throughput, better performance for full/archive node syncing, better client diversity from GETH, etc. I see Uniswap switching from GETH to RETH for high up-time full-node use case will bring positive effect to those long-term benefits overtime. |
jsy1218
force-pushed
the
jsy1218/route-168-implement-quicknode-geth-vs-reth-output-comparison
branch
from
283d9e7
to
d66b28e
Compare
Paradigm RETH just realized production read 1.0.0. QuickNode is ready for us to consume the production-ready dedicated RETH cluster as well. Before we switch over, we will need to ensure every RPC output from RETH client is the same as QuickNode GETH client.
I will write the unit test as part of this PR as well.