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

soroban-rpc: add a flag to launch core with diagnostic events #474

Closed
tsachiherman opened this issue Mar 6, 2023 · 17 comments
Closed

soroban-rpc: add a flag to launch core with diagnostic events #474

tsachiherman opened this issue Mar 6, 2023 · 17 comments
Assignees

Comments

@tsachiherman
Copy link
Contributor

tsachiherman commented Mar 6, 2023

What

soroban-rpc: add a flag to launch core with debug events

Add an integration test to cover launching core with new flag

Epic

#471

@2opremio
Copy link
Contributor

From stellar/rs-soroban-env#713 I don't think Core needs to be started up in a special way to enable Diagnostic Evets.

@tsachiherman what makes you think that?

@tsachiherman
Copy link
Contributor Author

Given that debug events are unmetered, I think that they have to be disabled by default. Otherwise, everyone would be using these instead of regular events, right ?

@2opremio
Copy link
Contributor

I am not sure, but I haven’t seen any flags in core.

@sisuresh can you confirm?

@sisuresh
Copy link
Contributor

Core will have a config setting to enable debug events. The core commit linked in stellar/rs-soroban-env#713 is just incomplete.

@2opremio
Copy link
Contributor

Ah, ok, thanks @sisuresh . Then, this ticket will entail. Either:

A:

  • Adding a new flag to soroban-rpc
  • Modify the captive-core backend library in the Go monorepo to translate the flag into a new Core config option

B:

  • Simply add a new config parameter to the captive-core config file which enables the flag (and in turn results in the generated config flag containing the config option). This needs to be done in the Core monorepo.

@tsachiherman
Copy link
Contributor Author

Given that the soroban-rpc needs to know this option is enabled in order to process the requests, I think that option A is the better one. It also "hides" the core implementation details, which is great.

@2opremio
Copy link
Contributor

2opremio commented Mar 13, 2023

soroban-rpc already needs to supply a captive-core configuration file (and so does Horizon)

@tsachiherman tsachiherman changed the title soroban-rpc: add a flag to launch core with debug events soroban-rpc: add a flag to launch core with diagnostic events Mar 15, 2023
@paulbellamy
Copy link
Contributor

To enable diagnostic events, core needs to be launched with this in the config file:
ENABLE_SOROBAN_DIAGNOSTIC_EVENTS=true

@paulbellamy
Copy link
Contributor

Given that the soroban-rpc needs to know this option is enabled in order to process the requests, I think that option A is the better one. It also "hides" the core implementation details, which is great.

Soroban-rpc actually always needs to use the txmeta to auto-detect whether diagnostic events are enabled.

The precedent from horizon is that the toml file overrides cli flags (and that we prefer users using the toml file, to prevent flag-explosion). So the toml file might enable/disable diagnostic events either way.

Fortunately, it's easy to auto-detect (as soon as we have ingested events), because if diagnostic events are enabled, the diagnosticEvents field in the txmeta will contain all the events. And if it isn't, that field will be empty. The other way to detect (before ingesting) would be to load the captive-core toml file, and check the setting.

@sreuland
Copy link
Contributor

@paul, I think your suggestion is to then follow option b for now? and diagnostic events flag would ultimately be processed through the upcoming toml suggestion - #507 for productionalization as how deployments set it externally, correct?

@paulbellamy
Copy link
Contributor

paulbellamy commented Mar 20, 2023

Yes, I'm saying option b, for now. But nothing to do with #507 (for now)

ENABLE_SOROBAN_DIAGNOSTIC_EVENTS=true would be set in the file passed to: --captive-core-config-path

@github-project-automation github-project-automation bot moved this from In Progress to Done in Platform Scrum Mar 28, 2023
@paulbellamy
Copy link
Contributor

Fixed in stellar/quickstart#435

@orbitlens
Copy link

Got an error

[default FATAL] Got an exception: Failed to parse './futurenet.cfg' :Unknown configuration entry: 'ENABLE_SOROBAN_DIAGNOSTIC_EVENTS'

I built stellar-core from sources (soroban-preview-10-July16 branch) and it returns me this error whenever I'm trying to launch stellar-core to join futurenet.

Is this option currently available only for the captive core? And if yes, could you please fix this?

@tsachiherman
Copy link
Contributor Author

tsachiherman commented Aug 15, 2023 via email

@orbitlens
Copy link

orbitlens commented Aug 15, 2023

Did you compile soroban-core with soroban support ? There is a configure flag that makes that happen.

No, I didn't. Thought that the branch with the name soroban-preview-10-July16 has Soroban support turned on by default.

@tsachiherman Could you please point me in the right direction? How can I set this flag?
My compilation pipeline currently looks like this:

#bash
./autogen.sh
./configure
make -j 4

@tsachiherman
Copy link
Contributor Author

Did you compile soroban-core with soroban support ? There is a configure flag that makes that happen.

No, I didn't. Thought that the branch with the name soroban-preview-10-July16 has Soroban support turned on by default.

@tsachiherman Could you please point me in the right direction? How can I set this flag? My compilation pipeline currently looks like this:

#bash
./autogen.sh
./configure
make -j 4

I was always compiling core with the following:

make clean && ./autogen.sh && ./configure --disable-tests --enable-next-protocol-version-unsafe-for-production && make -j8

@orbitlens
Copy link

@tsachiherman thanks for the info. It works.
🤝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants