Skip to content
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

rpc: Add diagnostic events to sendTransaction response #1152

Merged
merged 5 commits into from
Jan 8, 2024

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Jan 4, 2024

What

Add new diagnosticEventsXdr field to sendTransaction's RPC method response.

Closes #1157

Similar to stellar/go#5148

Also, use the new Core ENABLE_DIAGNOSTICS_FOR_TX_SUBMISSION flag in integration tests.

Why

Improve debug-ability of Soroban transaction submission.

Known limitations

N/A

@2opremio 2opremio enabled auto-merge (squash) January 8, 2024 17:22
@@ -1,4 +1,5 @@
ARTIFICIALLY_ACCELERATE_TIME_FOR_TESTING=true
ENABLE_DIAGNOSTICS_FOR_TX_SUBMISSION=true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this core cfg needs to be added to rpc's captive core instance for any deployment correct? I assume is the case, we'll need to update the rpc deployment manifests on kube to include this and i spun up quickstart/#543

Copy link
Contributor Author

@2opremio 2opremio Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn’t need to be captive core specifically. It should be added to any core instance you are sending transactions to, and only if the more general ENABLE_SOROBAN_DIAGNOSTIC_EVENTS isn’t already enabled (@dmkozh please confirm whether this is correct)

Edit: read @dmkozh ’s comment below for a full description

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only if the more general ENABLE_SOROBAN_DIAGNOSTIC_EVENTS isn’t already enabled

These flags are independent and control different things. Here is the brief description:

ENABLE_SOROBAN_DIAGNOSTIC_EVENTS:

  • Adds the apply-time diagnostic events to the tx meta stream
  • Meaningless if tx meta is not enabled and is not allowed at all on validator nodes
  • Should be enabled for the captive core nodes that are being used for tx meta ingestion

ENABLE_DIAGNOSTICS_FOR_TX_SUBMISSION

  • Adds the diagnostics directly to the Core's tx endpoint JSON response.
  • The diagnostics are only populated for the transactions that get immediately rejected by Core (i.e. they don't have a chance to be applied)
  • Can be enabled on any node, including validators
  • Should be enabled on the nodes which accept transactions (from Horizon/soroban-rpc). Typically these are not captive core nodes, but in some setups I can imagine both flags being enabled (notably, for standalone network mode both flags are useful).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmkozh , awesome, thanks for further clarification, yes, in some deployment configs, the captive core instance is the node used for tx sub. btw, is there a separate core cfg variables doc that has this same content for ref?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like we have a doc like that, but we have inline comments for most of the configuration flags in the example config (https://github.com/stellar/stellar-core/blob/f9999b8d31aef2b3e96226685b55a8506ba3c721/docs/stellar-core_example.cfg#L610). I'll open an issue to update it with the new soroban-related flags.

Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, left comment on thoughts for propagating the new api field to upstream(docs, sdks).

@2opremio 2opremio merged commit d1f1fa9 into stellar:main Jan 8, 2024
24 checks passed
@2opremio 2opremio deleted the sendtransaction-diag-events branch January 8, 2024 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rpc: Add diagnostic events to sendTransaction method response
3 participants