Accumulate sequentially when ecalli=trace is enabled

[tomusdrw]

No description provided.

[coderabbitai]

📝 Walkthrough

Summary by CodeRabbit

• New Features

  • Trace logging now automatically enforces sequential execution mode regardless of configuration settings
  • Added warning notifications when trace logging overrides user-configured parallel execution preferences

• Refactor

  • Refactored trace availability detection mechanism for improved maintainability
  • Extended public module exports to include the trace logging component

Walkthrough

Three files are modified to support trace logging integration with accumulation logic. A new static helper method isTraceEnabled() is added to EcalliTraceLogger to encapsulate trace availability checks. The EcalliTraceLogger is re-exported from the executor module. The Accumulate class gains a public options field and implements logic to force sequential accumulation mode when trace logging is enabled, with corresponding warning messages for this behavior change.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

A helper is born to check traces with care, 🐰
Sequential flows keep logs fair and square,
Three files now dance in harmonious way,
Tracing the path where the code runs to play!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to evaluate whether it relates to the changeset.	Add a pull request description explaining the purpose and rationale for forcing sequential accumulation when ecalli trace is enabled.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: forcing sequential accumulation when ecalli trace logging is enabled, which is the core logic change in the Accumulate class.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch td-ecalli-trace-sequential

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

packages/jam/transition/accumulate/accumulate.ts (1)

89-99: Move option resolution/logging out of the constructor.

Line 91–Line 99 introduces branching and logging in a non-private constructor. Please shift this logic into a static builder/factory and keep the constructor assignment-only.

Proposed refactor sketch

 export class Accumulate {
   public readonly options: AccumulateOptions;

-  constructor(
+  private constructor(
     public readonly chainSpec: ChainSpec,
     public readonly blake2b: Blake2b,
     public readonly state: AccumulateState,
-    options: AccumulateOptions,
+    options: AccumulateOptions,
   ) {
-    const ecalliTraceEnabled = EcalliTraceLogger.isTraceEnabled();
-    const accumulateSequentially = options.accumulateSequentially === true || ecalliTraceEnabled;
-    this.options = { ...options, accumulateSequentially };
-
-    if (ecalliTraceEnabled && options.accumulateSequentially !== true) {
-      logger.warn`⚠️ ecalli trace logging is enabled: forcing sequential accumulation to keep the trace output ordered.`;
-    } else if (accumulateSequentially) {
-      logger.warn`⚠️ Parallel accumulation is disabled. Running in sequential mode.`;
-    }
+    this.options = options;
   }
+
+  static create(
+    chainSpec: ChainSpec,
+    blake2b: Blake2b,
+    state: AccumulateState,
+    options: AccumulateOptions,
+  ): Accumulate {
+    const ecalliTraceEnabled = EcalliTraceLogger.isTraceEnabled();
+    const accumulateSequentially = options.accumulateSequentially === true || ecalliTraceEnabled;
+    const resolvedOptions = { ...options, accumulateSequentially };
+
+    if (ecalliTraceEnabled && options.accumulateSequentially !== true) {
+      logger.warn`⚠️ ecalli trace logging is enabled: forcing sequential accumulation to keep the trace output ordered.`;
+    } else if (accumulateSequentially) {
+      logger.warn`⚠️ Parallel accumulation is disabled. Running in sequential mode.`;
+    }
+
+    return new Accumulate(chainSpec, blake2b, state, resolvedOptions);
+  }
 }

As per coding guidelines, "For constructors, avoid logic inside non-private constructors: constructors should just assign fields (logic goes into static builder methods), and avoid constructor overloading."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/jam/transition/accumulate/accumulate.ts` around lines 89 - 99, The
constructor currently computes accumulateSequentially, mutates this.options and
emits logs (using EcalliTraceLogger and logger) — move that branching and
logging out of the non-private constructor into a new static factory method
(e.g., Accumulate.create or Accumulate.fromOptions) that accepts
AccumulateOptions, computes ecalliTraceEnabled and accumulateSequentially, logs
the appropriate warnings, and then calls a private constructor that only assigns
fields (this.options = ...). Ensure references to EcalliTraceLogger,
options.accumulateSequentially, and logger.warn are moved to the static method
and the public constructor becomes assignment-only.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/jam/transition/accumulate/accumulate.ts`:
- Around line 89-99: The constructor currently computes accumulateSequentially,
mutates this.options and emits logs (using EcalliTraceLogger and logger) — move
that branching and logging out of the non-private constructor into a new static
factory method (e.g., Accumulate.create or Accumulate.fromOptions) that accepts
AccumulateOptions, computes ecalliTraceEnabled and accumulateSequentially, logs
the appropriate warnings, and then calls a private constructor that only assigns
fields (this.options = ...). Ensure references to EcalliTraceLogger,
options.accumulateSequentially, and logger.warn are moved to the static method
and the public constructor becomes assignment-only.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b540417e-7511-42b8-9874-4f2ec960f801

📥 Commits

Reviewing files that changed from the base of the PR and between 9ffd7ac and 3f511ba.

📒 Files selected for processing (3)

• packages/core/pvm-host-calls/ecalli-trace-logger.ts
• packages/jam/executor/index.ts
• packages/jam/transition/accumulate/accumulate.ts

[github-actions]

View all

File	Benchmark	Ops
bytes/hex-from.ts[0]	parse hex using Number with NaN checking	63562.11 ±3.46%	85.82% slower
bytes/hex-from.ts[1]	parse hex from char codes	448163.89 ±1.1%	fastest ✅
bytes/hex-from.ts[2]	parse hex from string nibbles	255396.03 ±0.62%	43.01% slower
bytes/hex-to.ts[0]	number toString + padding	85474.26 ±1.34%	fastest ✅
bytes/hex-to.ts[1]	manual	6529.28 ±1.16%	92.36% slower
codec/bigint.compare.ts[0]	compare custom	85876847.86 ±4.31%	fastest ✅
codec/bigint.compare.ts[1]	compare bigint	80947364.07 ±4.65%	5.74% slower
codec/bigint.decode.ts[0]	decode custom	55609519.02 ±8.2%	26.81% slower
codec/bigint.decode.ts[1]	decode bigint	75977558.79 ±5.99%	fastest ✅
codec/decoding.ts[0]	manual decode	8237498.38 ±2.88%	85.55% slower
codec/decoding.ts[1]	int32array decode	55553761.01 ±3.85%	2.58% slower
codec/decoding.ts[2]	dataview decode	57024808.88 ±4.2%	fastest ✅
codec/encoding.ts[0]	manual encode	901750.9 ±2.36%	13.33% slower
codec/encoding.ts[1]	int32array encode	1039072.13 ±2.05%	0.14% slower
codec/encoding.ts[2]	dataview encode	1040500.68 ±1.79%	fastest ✅
collections/map-set.ts[0]	2 gets + conditional set	66039.94 ±1.21%	fastest ✅
collections/map-set.ts[1]	1 get 1 set	42501.49 ±0.9%	35.64% slower
logger/index.ts[0]	console.log with string concat	5273035.55 ±18.2%	fastest ✅
logger/index.ts[1]	console.log with args	872501.86 ±68.25%	83.45% slower
math/add_one_overflow.ts[0]	add and take modulus	62376573.76 ±46.19%	18.21% slower
math/add_one_overflow.ts[1]	condition before calculation	76262634.55 ±6.48%	fastest ✅
math/count-bits-u32.ts[0]	standard method	37113888.66 ±2.88%	50.68% slower
math/count-bits-u32.ts[1]	magic	75250499.45 ±5.64%	fastest ✅
math/count-bits-u64.ts[0]	standard method	393603.85 ±0.54%	86.93% slower
math/count-bits-u64.ts[1]	magic	3011126.8 ±0.32%	fastest ✅
math/mul_overflow.ts[0]	multiply and bring back to u32	99448450.9 ±4.98%	fastest ✅
math/mul_overflow.ts[1]	multiply and take modulus	97284379.42 ±5.93%	2.18% slower
math/switch.ts[0]	switch	97464721.31 ±5.96%	fastest ✅
math/switch.ts[1]	if	96367414.01 ±5.19%	1.13% slower
hash/index.ts[0]	hash with numeric representation	67.25 ±0.93%	28.37% slower
hash/index.ts[1]	hash with string representation	42.95 ±0.53%	54.25% slower
hash/index.ts[2]	hash with symbol representation	64.04 ±1.35%	31.79% slower
hash/index.ts[3]	hash with uint8 representation	73.07 ±0.73%	22.17% slower
hash/index.ts[4]	hash with packed representation	93.89 ±0.32%	fastest ✅
hash/index.ts[5]	hash with bigint representation	75.33 ±0.38%	19.77% slower
hash/index.ts[6]	hash with uint32 representation	90.06 ±0.42%	4.08% slower
bytes/bytes-to-number.ts[0]	Conversion with bitops	3297 ±5.4%	fastest ✅
bytes/bytes-to-number.ts[1]	Conversion without bitops	2793.6 ±3.83%	15.27% slower
bytes/compare.ts[0]	Comparing Uint32 bytes	12163.93 ±0.43%	0.21% slower
bytes/compare.ts[1]	Comparing raw bytes	12189.31 ±0.82%	fastest ✅
codec/view_vs_collection.ts[0]	Get first element from Decoded	15014.51 ±0.47%	53.47% slower
codec/view_vs_collection.ts[1]	Get first element from View	32267.4 ±1.35%	fastest ✅
codec/view_vs_collection.ts[2]	Get 50th element from Decoded	14959.51 ±0.55%	53.64% slower
codec/view_vs_collection.ts[3]	Get 50th element from View	17069.79 ±0.76%	47.1% slower
codec/view_vs_collection.ts[4]	Get last element from Decoded	14334.73 ±1.31%	55.58% slower
codec/view_vs_collection.ts[5]	Get last element from View	11480.56 ±1.34%	64.42% slower
codec/view_vs_object.ts[0]	Get the first field from Decoded	193281.14 ±0.92%	0.79% slower
codec/view_vs_object.ts[1]	Get the first field from View	50271.76 ±0.47%	74.2% slower
codec/view_vs_object.ts[2]	Get the first field as view from View	48825.56 ±0.6%	74.94% slower
codec/view_vs_object.ts[3]	Get two fields from Decoded	194823.34 ±0.72%	fastest ✅
codec/view_vs_object.ts[4]	Get two fields from View	38828.72 ±1.37%	80.07% slower
codec/view_vs_object.ts[5]	Get two fields from materialized from View	79093.71 ±0.62%	59.4% slower
codec/view_vs_object.ts[6]	Get two fields as views from View	35512.47 ±2.51%	81.77% slower
codec/view_vs_object.ts[7]	Get only third field from Decoded	176785.92 ±2.75%	9.26% slower
codec/view_vs_object.ts[8]	Get only third field from View	47264.83 ±1.75%	75.74% slower
codec/view_vs_object.ts[9]	Get only third field as view from View	42746.12 ±2.38%	78.06% slower
collections/map_vs_sorted.ts[0]	Map	136323.77 ±0.3%	fastest ✅
collections/map_vs_sorted.ts[1]	Map-array	47487.83 ±0.09%	65.17% slower
collections/map_vs_sorted.ts[2]	Array	55189.94 ±0.24%	59.52% slower
collections/map_vs_sorted.ts[3]	SortedArray	82947.48 ±0.65%	39.15% slower
collections/hash-dict-vs-blob-dict_delete.ts[0]	StringHashDictionary	2279.14 ±2.58%	fastest ✅
collections/hash-dict-vs-blob-dict_delete.ts[1]	BlobDictionary(1)	2184.49 ±2.89%	4.15% slower
collections/hash-dict-vs-blob-dict_delete.ts[2]	BlobDictionary(2)	2229.52 ±2.63%	2.18% slower
collections/hash-dict-vs-blob-dict_delete.ts[3]	BlobDictionary(3)	2197.4 ±2.46%	3.59% slower
collections/hash-dict-vs-blob-dict_delete.ts[4]	BlobDictionary(4)	2103.66 ±3.99%	7.7% slower
collections/hash-dict-vs-blob-dict_delete.ts[5]	BlobDictionary(5)	2184.8 ±3.32%	4.14% slower
collections/hash-dict-vs-blob-dict_get.ts[0]	StringHashDictionary	2998.77 ±0.41%	0.43% slower
collections/hash-dict-vs-blob-dict_get.ts[1]	BlobDictionary(1)	2996.26 ±0.6%	0.51% slower
collections/hash-dict-vs-blob-dict_get.ts[2]	BlobDictionary(2)	3011.66 ±0.52%	fastest ✅
collections/hash-dict-vs-blob-dict_get.ts[3]	BlobDictionary(3)	2899.73 ±1.72%	3.72% slower
collections/hash-dict-vs-blob-dict_get.ts[4]	BlobDictionary(4)	2944.58 ±1.49%	2.23% slower
collections/hash-dict-vs-blob-dict_get.ts[5]	BlobDictionary(5)	2890.31 ±1.19%	4.03% slower
collections/hash-dict-vs-blob-dict_set.ts[0]	StringHashDictionary	2244 ±0.76%	4.36% slower
collections/hash-dict-vs-blob-dict_set.ts[1]	BlobDictionary(1)	2036.6 ±2.18%	13.2% slower
collections/hash-dict-vs-blob-dict_set.ts[2]	BlobDictionary(2)	1921.93 ±3.82%	18.08% slower
collections/hash-dict-vs-blob-dict_set.ts[3]	BlobDictionary(3)	1885.39 ±3.1%	19.64% slower
collections/hash-dict-vs-blob-dict_set.ts[4]	BlobDictionary(4)	2199.41 ±2.44%	6.26% slower
collections/hash-dict-vs-blob-dict_set.ts[5]	BlobDictionary(5)	2346.18 ±0.41%	fastest ✅
hash/blake2b.ts[0]	our hasher	1.03 ±3.56%	fastest ✅
hash/blake2b.ts[1]	blake2b js	0.03 ±3.75%	97.09% slower
crypto/ed25519.ts[0]	native crypto	5.43 ±1.62%	fastest ✅
crypto/ed25519.ts[1]	wasm lib	2.17 ±0.72%	60.04% slower
crypto/ed25519.ts[2]	wasm lib batch	2.1 ±2.03%	61.33% slower

Benchmarks summary: 83/83 OK ✅