Improving scripts used to regenerate the archive data #17520

dannywillems · 2025-07-11T13:42:33Z

PG_DB=archive_test make postgres-setup
PG_DB=archive_test TOTAL_BLOCKS=2 make regenerate-archive

still exists with an error, but it seems to come from the replayer script. I'll investigate later.

The reviewer can check commit by commit.
The Makefile target can be used to use any user, database name or PostgreSQL instance.

dannywillems · 2025-07-14T09:33:38Z

!ci-build-me

Copilot

Pull Request Overview

This PR enhances portability and configurability of the archive-regeneration process by parameterizing PostgreSQL connection settings, tightening shell options, and updating documentation.

Add strict shell flags (set -euo pipefail) and parameterize PG connection in all scripts
Replace hard-coded psql/dropdb/createdb calls with PGPASSWORD-backed calls and unified env vars
Introduce PG_* variables and new Makefile targets for PostgreSQL setup, login, and cleanup; update README formatting

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
scripts/regenerate-archive.sh	Parameterize PG creds, tighten shell flags, improve quoting
scripts/mina-local-network/split_precomputed_log.sh	Add strict shell flags, parameterize PG vars, psql refactor
scripts/mina-local-network/mina-local-network.sh	Tighten shell flags, parameterize PG creds, refactor echo/printf
scripts/mina-local-network/README.md	Wrap long list items for consistent Markdown rendering
Makefile	Define `PG_*` variables, add `postgres-setup`, `postgres-clean`, `regenerate-archive` targets

Comments suppressed due to low confidence (2)

scripts/mina-local-network/split_precomputed_log.sh:11

[nitpick] The environment variable for password is split across PG_PW and PG_PASSWD, which can be confusing. Consider using a single name (PG_PW or PG_PASSWORD) consistently.

PG_PASSWD=${PG_PW:-""}

scripts/regenerate-archive.sh:42

The -pd flag is passed to mina-local-network.sh to set the database, but there’s no evidence the script parses -pd. Ensure the CLI parsing supports this option or correct it.

    -pd "${PG_DB}" \

scripts/regenerate-archive.sh

scripts/mina-local-network/split_precomputed_log.sh

Copilot · 2025-07-14T11:26:14Z

Makefile

+	@sudo -u postgres createuser -d -r -s $(PG_USER) 2>/dev/null || true
+	@sudo -u postgres psql -c "ALTER USER $(PG_USER) PASSWORD '$(PG_PW)'" 2>/dev/null || true
+	@sudo -u postgres createdb -O $(PG_USER) $(PG_DB) 2>/dev/null || true
+	@sudo -u postgres createdb -O $(PG_USER) $(PG_USER) 2>/dev/null || true


This target creates a database named after the user twice ($(PG_USER) as both DB name and owner). It seems unintended; remove or clarify the second createdb invocation.

Suggested change

@sudo -u postgres createdb -O $(PG_USER) $(PG_USER) 2>/dev/null || true

No, it is expected. We must ensure that there is the default database created for the user, which uses, by convention, the username as the database name.

Could we extract this feature into archive node, given people running archive node may want this feature?

And, it doesn't seemed to set up a schema in the DB here.

scripts/mina-local-network/mina-local-network.sh

scripts/regenerate-archive.sh

dannywillems · 2025-07-14T13:39:22Z

!ci-nightly-me

dannywillems · 2025-07-14T13:39:34Z

!ci-nightly-me

dannywillems · 2025-07-14T16:12:47Z

!ci-nightly-me

dannywillems · 2025-07-14T16:55:57Z

Build: https://buildkite.com/o-1-labs-2/mina-end-to-end-nightlies/builds/3800/steps/canvas

Using postgresql everywhere, even though both are accepted.

dannywillems · 2025-07-17T18:51:46Z

!ci-build-me

glyh

Minor quality improvement request

glyh · 2025-07-18T00:11:00Z

Makefile

+	@sudo -u postgres createuser -d -r -s $(PG_USER) 2>/dev/null || true
+	@sudo -u postgres psql -c "ALTER USER $(PG_USER) PASSWORD '$(PG_PW)'" 2>/dev/null || true
+	@sudo -u postgres createdb -O $(PG_USER) $(PG_DB) 2>/dev/null || true
+	@sudo -u postgres createdb -O $(PG_USER) $(PG_USER) 2>/dev/null || true


Could we extract this feature into archive node, given people running archive node may want this feature?

glyh · 2025-07-18T00:11:43Z

Makefile

+	@sudo -u postgres createuser -d -r -s $(PG_USER) 2>/dev/null || true
+	@sudo -u postgres psql -c "ALTER USER $(PG_USER) PASSWORD '$(PG_PW)'" 2>/dev/null || true
+	@sudo -u postgres createdb -O $(PG_USER) $(PG_DB) 2>/dev/null || true
+	@sudo -u postgres createdb -O $(PG_USER) $(PG_USER) 2>/dev/null || true


And, it doesn't seemed to set up a schema in the DB here.

glyh · 2025-07-18T00:13:15Z

scripts/mina-local-network/mina-local-network.sh

+  REST_PORT=$((BASE_PORT + 1))
+  EXTERNAL_PORT=$((BASE_PORT + 2))
+  DAEMON_METRICS_PORT=$((BASE_PORT + 3))
+  LIBP2P_METRICS_PORT=$((BASE_PORT + 4))


It'll port clash if some port is occupied by another program. I'd use a random port that's unused if possible.

Or at least this is a good place to fail gracefully by checking if any ports are occupied

glyh · 2025-07-18T00:16:09Z

scripts/mina-local-network/mina-local-network.sh


  ARCHIVE_ADDRESS_CLI_ARG="-archive-address ${ARCHIVE_SERVER_PORT}"
 fi

 # ================================================
 # Configure the Seed Peer ID

-SEED_PEER_ID="/ip4/127.0.0.1/tcp/$((${SEED_START_PORT} + 2))/p2p/12D3KooWAFFq2yEQFFzhU5dt64AWqawRuomG9hL8rSmm5vxhAsgr"
+SEED_PEER_ID="/ip4/127.0.0.1/tcp/$((SEED_START_PORT + 2))/p2p/12D3KooWAFFq2yEQFFzhU5dt64AWqawRuomG9hL8rSmm5vxhAsgr"


What is this port calculation and this magic address?

glyh · 2025-07-18T00:17:02Z

scripts/mina-local-network/mina-local-network.sh

  mkdir -p "${FOLDER}"
-  spawn-node "${FOLDER}" $((${WHALE_START_PORT} + (${i} * 5))) -peer ${SEED_PEER_ID} -block-producer-key ${KEY_FILE} \
-    -libp2p-keypair "${LEDGER_FOLDER}"/libp2p_keys/whale_${i} "${ARCHIVE_ADDRESS_CLI_ARG}"
+  spawn-node "${FOLDER}" $((WHALE_START_PORT + (i * 5))) \


I have no idea why we're calculating ports like this here

dannywillems · 2025-07-18T18:23:20Z

Minor quality improvement request

I do not disagree with the requested changes, but I would prefer to achieve this in a follow-up PR. It's out of the context of this patch, which was revamping the script and making it more customizable (different db name, user, password). In addition to that, it fixes the ignored shellcheck warnings.

glyh

Feel free to do follow-up PRs, then

dannywillems requested review from a team as code owners July 11, 2025 13:42

dannywillems marked this pull request as draft July 11, 2025 13:42

dannywillems force-pushed the dw/misc-mina-local-network-script branch 8 times, most recently from f193da9 to 9b0861e Compare July 11, 2025 17:59

dannywillems changed the title ~~WIP - suggestion~~ Improving scripts used to regenerate the archive data Jul 11, 2025

dannywillems marked this pull request as ready for review July 11, 2025 18:02

Base automatically changed from dkijania/regenerate_archive_data to compatible July 14, 2025 08:38

dannywillems force-pushed the dw/misc-mina-local-network-script branch 5 times, most recently from e37480d to 405b134 Compare July 14, 2025 09:33

dannywillems mentioned this pull request Jul 14, 2025

Makefile: add targets to setup, clean and login to PostgreSQL #17519

Closed

dannywillems requested a review from Copilot July 14, 2025 11:24

Copilot AI reviewed Jul 14, 2025

View reviewed changes

dannywillems force-pushed the dw/misc-mina-local-network-script branch from a84ae41 to c05aef3 Compare July 14, 2025 13:33

dannywillems force-pushed the dw/misc-mina-local-network-script branch from 0606eee to 1b57088 Compare July 14, 2025 13:39

dannywillems force-pushed the dw/misc-mina-local-network-script branch from 95a751d to 26f1537 Compare July 14, 2025 16:12

dannywillems added 25 commits July 17, 2025 20:51

mina-local-network: delete trailing whitespaces

bda3b83

mina-local-network: double quote LEDGER_FOLDER for shellcheck

414fdfd

regenerate-archive: use PG variables for PG related commands

e0705e7

regenerate-archive: wrap long lines

38a9a5d

regenerate-archive: make TOTAL_BLOCKS value configurable

a6fa905

mina-local-network: add missing spaces

86843b2

mina-local-network: wrap between double quotes NODES_FOLDER var

b9f4424

mina-local-network: no need to expand numerical var in for loop

eb71307

mina-local-network: appease more shellcheck

e13b625

regenerate-archive: typo PG_PASSWD -> PG_PW

db3e5ef

split_precomputed_log: set common flags for bash scripts

969a2f3

split_precomputed_log: appease shellchek

09646fd

split_precomputed_log: use PG variables to allow different pg setup

9fcb72b

split_precomputed_log: make shellcheck totally happy

4c0cde6

mina-local-network: wrap at 80 chars

9afd923

mina-local-network: appease more shellcheck and wrap 80 chars

7d670c5

split_precomputed_log: set default value to precomputed_blocks dir

4c1ca05

mina-local-network: no need to expand in for loop

05e9105

mina-local-network: fix trap method to kill all background processes

38bc343

mina-local-network: use lowercase var for local var + wrap 80 chars

f65d59a

mina-local-network: remove unecessary ${} when performing arithmetic

127d3ed

scripts: double quotes to support special chars in passwords

ae76170

Unifying prefix for Postgres URI

9799360

Using postgresql everywhere, even though both are accepted.

split_precomputed_log: remove unused ARCHIVE_URI

996f754

Build/scripts: remove unused variable PG_URI

def82e8

dannywillems force-pushed the dw/misc-mina-local-network-script branch from 26f1537 to def82e8 Compare July 17, 2025 18:51

glyh requested changes Jul 18, 2025

View reviewed changes

glyh approved these changes Jul 19, 2025

View reviewed changes

Improving scripts used to regenerate the archive data #17520

Are you sure you want to change the base?

Improving scripts used to regenerate the archive data #17520

Uh oh!

Conversation

dannywillems commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dannywillems commented Jul 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dannywillems commented Jul 14, 2025

Uh oh!

dannywillems commented Jul 14, 2025

Uh oh!

dannywillems commented Jul 14, 2025

Uh oh!

dannywillems commented Jul 14, 2025

Uh oh!

dannywillems commented Jul 17, 2025

Uh oh!

glyh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannywillems commented Jul 18, 2025

Uh oh!

glyh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dannywillems commented Jul 11, 2025 •

edited

Loading