-
Notifications
You must be signed in to change notification settings - Fork 970
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
Use a separate flag for validation diagnostics in tx endpoint. #4104
Conversation
@@ -645,7 +670,7 @@ TransactionFrame::validateSorobanResources(SorobanNetworkConfig const& config, | |||
auto const& writeEntries = resources.footprint.readWrite; | |||
if (resources.instructions > config.txMaxInstructions()) | |||
{ | |||
pushSimpleDiagnosticError( | |||
pushValidationTimeDiagnosticError( |
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 config were to change after a transaction has been accepted to the mempool, that transaction would still not make it to apply time because if would fail validation when the transaction set is created right? This assumes the config was reduced to something that would invalidate the transactions.
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.
Hmm, I guess yes, we would attach the diagnostics to the transaction in this scenario, even though we don't return the result back to the user. But this seems like a very rare scenario, so probably it's negligible. The invariant of not attaching the diagnostics to meta will not be invalidated either (FWIW tx sets with invalid transactions can't be applied).
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.
FWIW tx sets with invalid transactions can't be applied
Yeah that's what I wanted to confirm. This sounds fine to me then.
src/main/Config.cpp
Outdated
@@ -264,6 +264,7 @@ Config::Config() : NODE_SEED(SecretKey::random()) | |||
MAX_DEX_TX_OPERATIONS_IN_TX_SET = std::nullopt; | |||
|
|||
ENABLE_SOROBAN_DIAGNOSTIC_EVENTS = false; | |||
ENABLE_DIAGNOSTICS_FOR_TX_SUBMISSION = 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.
Should this be enabled by default? Do we need the tier 1 validators to have this enabled? I was under the impression that transactions usually aren't submitted directly to tier 1. In that case, it'd be safer if this defaulted to false.
r+ 68dc8a53967ad6e024dd518265c2c35788974578 |
This flag seems safe to enable by default on validator nodes that accept transactions, as the messages have a reasonably small constant size. Without this, it's pretty much impossible to actually get diagnostics back, because validators don't have soroban diagnostics enabled, and watches don't normally accept transactions.
r+ 9befcc0 |
r+ 8f5242e |
Description
This flag seems safe to enable by default on validator nodes that accept transactions, as the messages have a reasonably small constant size. Without this, it's pretty much impossible to actually get diagnostics back, because validators don't have soroban diagnostics enabled, and watches don't normally accept transactions.
Checklist
clang-format
v8.0.0 (viamake format
or the Visual Studio extension)