Disable accounts.in_memory_only by default#8445
Disable accounts.in_memory_only by default#8445ripatel-fd wants to merge 4 commits intofiredancer-io:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR changes the default behavior of the accounts database configuration from in-memory-only mode to using persistent storage. The primary change is setting accounts.in_memory_only = false in the default configuration file, while refactoring test scripts to explicitly specify in-memory mode when needed.
Changes:
- Modified default configuration to disable
accounts.in_memory_only(changed fromtruetofalse) - Refactored test scripts to replace
--vinylflag with--in-memory-onlyflag - Updated test scripts to explicitly enable in-memory mode where needed
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/app/firedancer/config/default.toml | Changed default value of in_memory_only from true to false |
| src/flamenco/runtime/tests/run_ledger_backtest.sh | Replaced --vinyl flag with --in-memory-only and --accounts-db flags, restructured config generation logic |
| src/flamenco/runtime/tests/run_backtest_ci.sh | Replaced --vinyl flag with --in-memory-only in test invocations, removed outdated comments |
| src/flamenco/runtime/tests/run_backtest_all.sh | Replaced --vinyl flag with --in-memory-only in test invocations, added new test cases with explicit in-memory mode |
| contrib/test/test_firedancer_leader.sh | Added explicit in_memory_only = true to maintain existing behavior |
| contrib/test/run_solcap_tests.sh | Added explicit in_memory_only = true to maintain existing behavior |
| contrib/test/run_fd_shred_cap.sh | Added explicit in_memory_only = true to maintain existing behavior |
| contrib/offline-replay/offline_replay.toml | Added explicit in_memory_only = true to maintain existing behavior |
| opt | Added symlink to ../firedancer/opt |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f37a2a1 to
152eda4
Compare
152eda4 to
2e75d68
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2e75d68 to
fc93a17
Compare
fc93a17 to
8548663
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4073b4b to
04ebb9f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # If enabled, allocates [accounts.file_size_gib] memory and ignores | ||
| # [accounts.{cache_size_gib,max_unrooted_account_size_gib}]. | ||
| in_memory_only = true | ||
| in_memory_only = false |
There was a problem hiding this comment.
Switching accounts.in_memory_only to false changes the default storage mode to on-disk accounts and also makes [accounts.cache_size_gib,max_unrooted_account_size_gib] mandatory. Given the new very large file_size_gib default above, this can cause unexpected disk preallocation for configs that rely on defaults (e.g. devnet). If the intent is only to flip the default for mainnet, consider overriding in_memory_only=false in mainnet configs instead of changing the global default.
| in_memory_only = false | |
| in_memory_only = true |
| local rcpath; rcpath="$(rc_path "$pid")" | ||
| { | ||
| echo "$ret" | ||
| echo "$elapsed" | ||
| } > "$rcpath" |
There was a problem hiding this comment.
runner() writes results to rc_path, but the rc_path() helper was removed and is no longer defined anywhere in this script. With set -euo pipefail, this will fail with rc_path: command not found after the first test run. Either reintroduce rc_path() (and any associated RC handling) or remove the rc file write logic now that tests are run synchronously.
| dispatch "$OBJDIR"/unit-test/test_fddev | ||
| dispatch "$OBJDIR"/unit-test/test_firedancer_dev --testnet |
There was a problem hiding this comment.
dispatch() is invoking binaries under $OBJDIR/unit-test/..., but test_fddev and test_firedancer_dev are built via make-integration-test, which places them under $OBJDIR/integration-test/.... As written, dispatch will skip running the tests entirely (due to the -f check). Update the paths to the integration-test directory (or change the build rules to emit these under unit-test).
| dispatch "$OBJDIR"/unit-test/test_fddev | |
| dispatch "$OBJDIR"/unit-test/test_firedancer_dev --testnet | |
| dispatch "$OBJDIR"/integration-test/test_fddev | |
| dispatch "$OBJDIR"/integration-test/test_firedancer_dev --testnet |
| shift 1 | ||
|
|
||
| if [[ ! -f "$prog" ]]; then | ||
| return 0 |
There was a problem hiding this comment.
dispatch() returns success when the target binary does not exist. This can mask misconfigurations (or path bugs) and produce false-positive test runs. Consider failing with a clear error message when an expected test binary is missing, or at least emitting a warning and returning non-zero in CI.
| return 0 | |
| echo "test.sh: expected test binary not found: $prog" >&2 | |
| return 1 |
| make-unit-test = $(eval $(call _make-exe,$(1),$(2),$(3),unit-test,unit-test,$(4) $(LDFLAGS_EXE))) | ||
| run-unit-test = $(eval $(call _run-unit-test,$(1))) | ||
| make-integration-test = $(eval $(call _make-exe,$(1),$(2),$(3),integration-test,integration-test,$(4) $(LDFLAGS_EXE))) | ||
| run-integration-test = $(eval $(call _run-integration-test,$(1))) | ||
| make-fuzz-test = $(eval $(call _fuzz-test,$(1),$(2),$(3),$(4) $(LDFLAGS_EXE))) |
There was a problem hiding this comment.
run-integration-test macro was removed here, but the earlier usage/documentation block in this file still advertises $(call run-integration-test,name,args). To avoid confusion and broken expectations when adding tests, update the documentation to remove/replace that usage or reintroduce a supported macro.
| @@ -232,17 +232,6 @@ $(OBJDIR)/unit-test/automatic.txt: $(LOCAL_MKS) | |||
| $(RM) $@ | |||
| @$(foreach test,$(RUN_UNIT_TEST),echo $(test)>>$@;) | |||
|
|
|||
There was a problem hiding this comment.
The rule that generated $(OBJDIR)/integration-test/automatic.txt was removed, but make run-integration-test still passes --tests $(OBJDIR)/integration-test/automatic.txt (see this file earlier). This makes the integration-test runner dependent on a file that is no longer produced anywhere. Either restore generation of the integration test list, or update the run target/runner to not reference integration-test/automatic.txt.
| # Generate list of automatic integration tests from $(call run-integration-test,...) | |
| integration-test: $(OBJDIR)/integration-test/automatic.txt | |
| define _run-integration-test | |
| RUN_INTEGRATION_TEST+=$(OBJDIR)/integration-test/$(1) | |
| endef | |
| $(OBJDIR)/integration-test/automatic.txt: $(LOCAL_MKS) | |
| $(MKDIR) "$(OBJDIR)/integration-test" | |
| $(RM) $@ | |
| @$(foreach test,$(RUN_INTEGRATION_TEST),echo $(test)>>$@;) |
| max_accounts = 1_300_000_000 | ||
|
|
||
| # File size of the account database file (see [paths.accounts]). | ||
| # The entire account database is physically preallocated. | ||
| file_size_gib = 16 | ||
| file_size_gib = 800 |
There was a problem hiding this comment.
These new defaults (max_accounts = 1_300_000_000, file_size_gib = 800) drastically increase baseline resource requirements (index RAM + preallocated accounts DB). Because src/app/firedancer/config/devnet.toml does not override [accounts], running against devnet will now inherit these mainnet-scale defaults and likely fail on typical dev machines. Consider keeping smaller defaults and overriding only in mainnet configs, or add a devnet-specific [accounts] override with devnet-appropriate sizing.
No description provided.