You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Merge remote-tracking branch 'origin/main' into yperbasis/moksha
Five conflicts:
1) cmd/rpctest/rpctest/account_range_verify.go — main added defer
Close() on the two MDBX handles. Trivial: kept both.
2) db/migrations/migrations.go — main landed #21280 (the legacy E2
tables drop spinoff originally cut from this branch), which adds
dropLegacyE2Tables. Moksha has both dropLegacyE2Tables and
dropAccountIncarnation registered. Resolution: keep both in the
ChainDB sequence.
3) db/kv/kvcache/cache.go — main removed AssertCheckValues entirely
(the txpool stub call site went away with #21280's caller cleanup).
Moksha had it as a no-op stub. Take main's deletion; the only
reference left was a commented-out test line.
4) execution/stagedsync/exec3_finalize_test.go — PR #21211 deleted
~240 lines of test scenarios (TestFinalizeTx_SimpleTransfer,
_London, _AllScenarios, coinbaseIsRecipientScenario,
selfTransferScenario, hasCoinbaseDelta, adjustForTransferDelta)
because the finalizeWithIBS / finalizeTx (delta-args) code paths
they exercised were dead. Take main's deletion.
5) execution/state/intra_block_state.go — three blocks:
a) Same-block-revival check in CreateAccount. Main (#21319)
simplified it to `!account.Empty()` on the version-map-refreshed
record. Moksha had a path-by-path LatestTxIndex scan
(BalancePath/NoncePath/CodeHashPath > destructTxIndex). Take
main's cleaner check — semantically equivalent on the same
input (the refreshed `account` reflects exactly those higher-
txIndex writes).
b) prevInc / IncarnationPath bookkeeping for CreateAccount on
main. Entirely incarnation-dependent; moksha has no
IncarnationPath and no Account.Incarnation. Take HEAD's empty
resolution.
c) Synthetic versionRead(IncarnationPath, ...) in CreateAccount on
main. Same reason. Take HEAD's empty resolution.
Plus two non-conflicting touch-ups dropping IncarnationPath references
that the merge silently brought in via main-side changes:
- execution/state/versionmap.go MVReadResultNone/MapRead recursive-
cross-check whitelist dropped IncarnationPath from the path list.
- execution/state/versionedio.go SD short-circuit zero-value-fallback
whitelist dropped IncarnationPath from the path list.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy file name to clipboardExpand all lines: agents.md
+77-12Lines changed: 77 additions & 12 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -42,6 +42,43 @@ Before committing, always verify changes with: `make lint && make erigon integra
42
42
./build/bin/erigon --datadir=dev --chain=dev --beacon.api=beacon,validator,node,config # PoS dev mode
43
43
```
44
44
45
+
## Test-Driven Development
46
+
47
+
When fixing bugs or adding new features, follow the test-driven development (TDD) cycle: **Red → Green → Refactor**.
48
+
49
+
1.**Red** — write a failing test that specifies the desired behavior. Confirm it fails *for the right reason* (the behavior is missing or wrong), not because of a typo, missing import, or wrong setup.
50
+
2.**Green** — write the minimum production code needed to make the test pass. Do not write code the current failing test does not demand.
51
+
3.**Refactor** — clean up code and tests with the suite staying green. Skipping this step is how technical debt accumulates.
52
+
53
+
### For bug fixes
54
+
55
+
Reproduce the bug as a failing test **before** touching the fix. This proves three things at once: (a) the bug exists, (b) the agent/contributor understands it, and (c) the fix actually addresses it — the test flips red → green when the fix lands.
56
+
57
+
Never write the fix first and then "add a test for it" — that test only proves the code matches itself, not that it fixes the original defect. If the bug cannot be reproduced as a test, stop and either get more information or escalate; do not guess at a fix.
58
+
59
+
### For new features
60
+
61
+
- Drive the API shape from how the test wants to call it (outside-in).
62
+
- Start with the simplest meaningful behavior, not the full design.
63
+
- Add edge cases as separate Red → Green cycles, not bundled into one giant test.
64
+
65
+
### Anti-patterns
66
+
67
+
-**Test-after development**: writing code first, then "adding a test" — this is not TDD; the test merely echoes the code's existing behavior.
68
+
-**Tests that pass on the first run**: means the test did not actually drive anything; either the behavior already existed or the assertion is wrong.
69
+
-**Skipping the refactor step**: the suite is green but the design didn't improve.
70
+
-**Bundling many behaviors into one test**: makes failures hard to localize and refactors brittle.
71
+
-**Adding `t.Skip` instead of fixing a failing test**: forbidden for automated agents — see [Test skips](#test-skips) below.
72
+
73
+
### When pragmatism applies
74
+
75
+
TDD is the default for behavior changes (bug fixes, new logic, new endpoints). It applies less cleanly to:
76
+
- Pure refactors with no behavior change — existing tests are the safety net; do not write new tests just to satisfy the cycle.
77
+
- Exploratory spikes — throw the spike away and TDD the real implementation.
When skipping TDD for one of these reasons, say so explicitly in the PR description.
81
+
45
82
## Test skips
46
83
47
84
These rules apply project-wide — to every contributor and to every automated agent (LLM coding assistants, CI bots, etc.) working in this repository.
@@ -87,21 +124,49 @@ Run `make lint` before every push. The linter is non-deterministic — run it re
87
124
88
125
### Comments
89
126
90
-
Prefer self-explanatory code over comments. Use clear names and small, focused functions so the code reads on its own. Default to writing no comment.
127
+
**Default: no comment.** Clear names and small focused functions read on their own. The vast majority of code — including code written by automated agents — should carry zero new comments. Before adding one, ask whether renaming, extracting a helper, or restructuring would remove the need. Almost always, it does.
128
+
129
+
A comment may be warranted for: a non-obvious invariant the types don't enforce; a workaround for a bug in a dependency or the runtime (link the issue/commit); a surprising edge case a reader would otherwise miss; a performance-sensitive choice where the obvious implementation would be wrong.
130
+
131
+
When a comment is genuinely required, it MUST be:
132
+
133
+
-**One sentence; rarely two; never a paragraph.** No bulleted lists inside `//`. No multi-section block comments with `// Concurrency:` / `// Why:` / `// How to apply:` sub-headings. If the explanation doesn't fit in two sentences, the rest belongs in the commit message, the PR description, or a design doc — not in source, where it goes stale.
134
+
-**High-level, not scenario-specific.** Explain the invariant or gotcha in general terms. Don't walk through specific call sites, sequences of operations, or particular situations a reader could find with `grep`. The right level of abstraction is "what must remain true," not "what happened to me last Tuesday."
135
+
-**Free of forensic detail.** Strip dates (`// found on 2026-05-21`), devnet/branch names (`// seen on bal-devnet-7`), PR/issue/review references (`// flagged in #21314 round-4 review`), incident anecdotes, and "used by X, Y, Z" callsite lists. That history belongs in the commit message and PR description, where it survives intact; in source it rots, misleads later readers, and bloats the file.
136
+
-**Not a restatement of the code.** If a reader could delete the comment without losing information, delete it. Standard Go idioms and well-known library behavior don't need annotation.
137
+
138
+
A good comment:
139
+
140
+
```go
141
+
// Safe to close while read views are still iterating: the memStore backing
142
+
// makes Rollback a no-op on the data.
143
+
func(m *MemoryMutation) Rollback() { ... }
144
+
```
145
+
146
+
The same constraint written badly — long, name-dropping internal callers, threading review history through the source:
147
+
148
+
```go
149
+
// Rollback releases this mutation's local cursor cache and forwards Rollback
150
+
// to the backing in-memory tx / db. Concurrency invariant (load-bearing —
151
+
// see also Filters.WithOverlay): for a MemoryMutation created via
152
+
// NewMemoryBatch (the pure-Go memStore backing used by
153
+
// SharedDomains.blockOverlay), memTx.Rollback and memDb.Close are no-ops on
154
+
// the in-memory data — see memory_store.go. That is what makes it safe for
155
+
// the FCU bg-commit goroutine to call Close on the published BlockOverlay
156
+
// while concurrent RPC readers are still iterating views obtained via
157
+
// NewReadView / NewTemporalReadView. If this is ever switched to
158
+
// NewMemoryBatchMDBX (real MDBX backing, where Rollback DOES invalidate
159
+
// cursors), the bg-commit close + concurrent-RPC-reader pattern becomes
160
+
// unsafe and refcounting/drain logic is required...
161
+
```
162
+
163
+
If a constraint really needs to be enforced for the codebase's safety, prefer **code that enforces it** (a runtime assert, a type the caller can't misuse, a single private constructor) over a comment that describes it. A `panic` survives refactors; a long comment doesn't.
91
164
92
-
Add a comment only when the code itself can't tell the reader *why*:
93
-
- Workarounds for bugs in dependencies, the runtime, or other parts of the codebase (link the issue or commit when possible)
94
-
- Non-obvious invariants or constraints not enforced by types
95
-
- Surprising edge cases that are easy to miss when reading
96
-
- Performance-sensitive choices where the straightforward implementation would be wrong
165
+
Function docstrings follow the same rule: a one-line summary, plus param/return notes only when the signature doesn't already say it. A docstring that needs sections is a sign the function does too much, or the explanation belongs elsewhere.
97
166
98
-
Avoid:
99
-
- Restating what the code does (`// increment counter`)
100
-
- Referencing the current task, PR, or caller (`// added for the X flow`) — that belongs in the commit message
101
-
- Documenting standard Go idioms or well-known library behavior
102
-
-`// TODO` notes without a linked issue or owner
167
+
`// TODO` notes are only acceptable with a linked tracking issue and an owner. Better: file the issue and don't add the TODO; or fix it now.
103
168
104
-
When a comment is warranted, keep it short and focused on the *why*. If a reader could delete the comment without losing information, it shouldn't have been written.
169
+
**For automated agents specifically:** previous iterations of this guidance were not enough — agents kept producing multi-paragraph block comments enumerating call sites and incident history. Treat the rules above as hard limits. If you catch yourself writing a third sentence in a comment, stop and either delete it, condense to one sentence, or move the content into the commit message.
0 commit comments