internal/infrastructure/live-store/inmemory/offchain_txs.go (1)

31-36: Silently ignoring PSBT parse errors may cause inconsistent input tracking.

If psbt.NewFromRawBytes fails for a malformed checkpoint transaction, the inputs from that transaction won't be added to the inputs map. This could lead to inconsistent state where some inputs are tracked and others are not.

Consider propagating the error or at least logging the failure:

 for _, tx := range offchainTx.CheckpointTxs {
-	ptx, _ := psbt.NewFromRawBytes(strings.NewReader(tx), true)
+	ptx, err := psbt.NewFromRawBytes(strings.NewReader(tx), true)
+	if err != nil {
+		return fmt.Errorf("failed to parse checkpoint tx: %w", err)
+	}
 	for _, in := range ptx.UnsignedTx.TxIn {
 		m.inputs[in.PreviousOutPoint.String()] = struct{}{}
 	}
 }

The same pattern exists in Remove at line 50.

internal/infrastructure/live-store/redis/offchain_txs.go (2)

49-66: Retry loop ignores context cancellation.

The retry loop sleeps unconditionally without checking if the context has been cancelled. This means the operation will continue retrying even after the caller has given up.

Consider checking context before retrying:

 for range s.numOfRetries {
 	if err = s.rdb.Watch(ctx, func(tx *redis.Tx) error {
 		// ... pipeline logic ...
 	}); err == nil {
 		return nil
 	}
+	select {
+	case <-ctx.Done():
+		return fmt.Errorf("context cancelled while adding offchain tx %s: %w", offchainTx.ArkTxid, ctx.Err())
+	default:
+	}
 	time.Sleep(s.retryDelay)
 }

The same pattern applies to Remove at lines 94-108.

---

36-47: PSBT parse error silently ignored in Add.

Same issue as the in-memory implementation: if psbt.NewFromRawBytes fails, the inputs from that checkpoint transaction won't be tracked. This could lead to the Includes check returning incorrect results.

 for _, tx := range offchainTx.CheckpointTxs {
-	ptx, _ := psbt.NewFromRawBytes(strings.NewReader(tx), true)
+	ptx, err := psbt.NewFromRawBytes(strings.NewReader(tx), true)
+	if err != nil {
+		return fmt.Errorf("failed to parse checkpoint tx: %w", err)
+	}
 	for _, in := range ptx.UnsignedTx.TxIn {
 		inputs = append(inputs, in.PreviousOutPoint.String())
 	}
 }

internal/infrastructure/live-store/redis/boarding_inputs.go (2)

97-120: Variable shadowing: key reused in loop.

The loop variable key on line 106 shadows the outer key variable from line 100. Consider renaming to field for clarity.

-	for key, value := range values {
+	for field, value := range values {
 		rawSig := &sigsDTO{}
 		sig, err := rawSig.deserialize([]byte(value))
 		if err != nil {
-			return nil, fmt.Errorf("malformed signatures for input %s in storage: %v", key, err)
+			return nil, fmt.Errorf("malformed signatures for input %s in storage: %v", field, err)
 		}
-		inIndex, err := strconv.Atoi(key)
+		inIndex, err := strconv.Atoi(field)

---

122-125: Consider adding retry logic for consistency.

DeleteSignatures doesn't use retry logic unlike Set and AddSignatures. While Del is simpler, adding retry logic would make the error handling consistent across write operations.

internal/infrastructure/live-store/redis/round.go (1)

163-164: Unknown event types are silently skipped.

When an unknown eventType is encountered, the code silently continues without logging or returning an error. While this may be intentional for forward compatibility, it could mask data corruption or version mismatches. Consider adding a warning log to aid debugging.

 		default:
+			log.Warnf("unknown event type %d in round storage, skipping", eventType)
 			continue
 		}

internal/infrastructure/live-store/redis/tree_signing_session.go (1)

278-279: Silently ignoring Redis errors in watch goroutine.

Errors from HGetAll are discarded. While transient errors should allow the loop to continue, persistent failures will cause silent infinite polling. Consider logging the error.

-			noncesMap, _ := s.rdb.HGetAll(ctx, noncesKey).Result()
+			noncesMap, err := s.rdb.HGetAll(ctx, noncesKey).Result()
+			if err != nil {
+				log.Warnf("watchNoncesCollected: failed to get nonces: %v", err)
+				continue
+			}

The same pattern applies at Line 329 for sigsMap.

internal/core/application/service.go (2)

2275-2289: Inconsistent error handling on cache cleanup failures.

The cleanup operations in startRound log warnings/errors but continue execution. While this is reasonable for cleanup operations, the error messages reference existingRound.Id which could be empty or nil if this is the first round.

 	if err := s.cache.ForfeitTxs().Reset(ctx); err != nil {
-		log.WithError(err).Warnf(
-			"failed to delete forfeit txs from cache for round %s", existingRound.Id,
-		)
+		if existingRound != nil {
+			log.WithError(err).Warnf(
+				"failed to delete forfeit txs from cache for round %s", existingRound.Id,
+			)
+		} else {
+			log.WithError(err).Warn("failed to reset forfeit txs from cache")
+		}
 	}

Similar patterns apply to the subsequent cleanup calls.

---

3183-3188: Address the TODO comment.

There's a TODO about testing broadcast before updating storage. This ordering could lead to inconsistent state if the broadcast succeeds but subsequent storage updates fail. Consider implementing the suggested pattern or creating a tracking issue.

Do you want me to open a new issue to track this improvement?

internal/infrastructure/live-store/redis/intents.go (2)

271-292: Inconsistent error handling in Delete method.

The method returns errors from Get but ignores errors from SRem (lines 282, 289) and only logs errors from Delete (line 286). This inconsistency could leave orphaned entries in the vtxos set if Redis operations fail.

If the intent is idempotent deletion, consider at least logging the SRem failures.

 		for _, vtxo := range intent.Inputs {
-			s.rdb.SRem(ctx, intentStoreVtxosKey, vtxo.Outpoint.String())
+			if err := s.rdb.SRem(ctx, intentStoreVtxosKey, vtxo.Outpoint.String()).Err(); err != nil {
+				log.Warnf("delete: failed to remove vtxo %s from set: %v", vtxo.Outpoint.String(), err)
+			}
 		}
 
 		if err := s.intents.Delete(ctx, id); err != nil {
 			log.Warnf("delete:failed to delete intent %s: %v", id, err)
 		}
 
-		s.rdb.SRem(ctx, intentStoreIdsKey, id)
+		if err := s.rdb.SRem(ctx, intentStoreIdsKey, id).Err(); err != nil {
+			log.Warnf("delete: failed to remove intent id %s from set: %v", id, err)
+		}

---

347-357: Consider returning error from IncludesAny.

The method logs errors but cannot propagate them to callers due to the interface signature. Callers cannot distinguish between "not found" and "Redis error". If this distinction matters, consider updating the interface to return an error.

internal/infrastructure/live-store/live_store_test.go (1)

433-550: Minor style inconsistency in context usage.

The test uses ctx := t.Context() for early operations (lines 434-448) but then uses t.Context() directly for later operations (lines 515+). This works correctly but is slightly inconsistent. Consider using the ctx variable throughout for consistency.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: louisinger
Repo: arkade-os/arkd PR: 686
File: internal/core/application/fraud.go:47-61
Timestamp: 2025-08-28T08:21:01.170Z
Learning: In reactToFraud function in internal/core/application/fraud.go, the goroutine that waits for confirmation and schedules checkpoint sweep should use context.Background() instead of the request context, as this is intentional design to decouple the checkpoint sweep scheduling from the request lifetime.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 686
File: internal/core/application/fraud.go:47-61
Timestamp: 2025-08-28T08:21:01.170Z
Learning: In reactToFraud function in internal/core/application/fraud.go, the goroutine that waits for confirmation and schedules checkpoint sweep should use context.Background() instead of the request context, as this is intentional design to decouple the checkpoint sweep scheduling from the request lifetime.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 722
File: pkg/ark-lib/intent/proof.go:52-58
Timestamp: 2025-09-29T14:33:52.871Z
Learning: In btcsuite/btcd's psbt package, the NewFromRawBytes function's boolean parameter (b64) automatically handles base64 decoding when set to true, so passing a base64 string via strings.NewReader with b64=true is the correct usage pattern. A common bug is manually base64-decoding the string and then passing b64=true, which causes a double-decode error.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 722
File: pkg/ark-lib/intent/proof.go:52-58
Timestamp: 2025-09-29T14:33:52.871Z
Learning: In btcsuite/btcd's psbt package, the NewFromRawBytes function's boolean parameter (b64) automatically handles base64 decoding when set to true, so passing a base64 string via strings.NewReader with b64=true is the correct usage pattern.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.

internal/core/application/service.go (2)

3344-3362: Error fetching selected intents still prevents RoundFailed propagation

On domain.RoundFailed you still abort event propagation when reading selected intents fails:

case domain.RoundFailed:
    intents, err := s.cache.Intents().GetSelectedIntents(ctx)
    if err != nil {
        log.WithError(err).Warn("failed to get selected intents")
        return
    }
    ...
    lastEvent = RoundFailed{ev, topics}

If the cache is temporarily unavailable, clients never see the RoundFailed event, which is more harmful than omitting topic details.

You can keep logging the error but fall back to an empty topic list:

- intents, err := s.cache.Intents().GetSelectedIntents(ctx)
- if err != nil {
-     log.WithError(err).Warn("failed to get selected intents")
-     return
- }
-
- topics := make([]string, 0, len(intents))
+ intents, err := s.cache.Intents().GetSelectedIntents(ctx)
+ if err != nil {
+     log.WithError(err).Warn("failed to get selected intents, propagating RoundFailed without topics")
+ }
+
+ topics := make([]string, 0, len(intents))
  for _, intent := range intents {
     ...

This preserves the critical failure signal while degrading gracefully on cache issues.

---

3330-3334: Connector index cache error still aborts RoundFinalizationStarted event propagation

On RoundFinalizationStarted you still return early if GetConnectorsIndexes fails:

connectorsIndex, err := s.cache.ForfeitTxs().GetConnectorsIndexes(ctx)
if err != nil {
    log.WithError(err).Warn("failed to get connectors index")
    return
}

This prevents connector tree events (and thus parts of round finalization info) from reaching clients, even though the main round events are available.

Consider logging and proceeding with an empty index to keep event propagation best‑effort:

- connectorsIndex, err := s.cache.ForfeitTxs().GetConnectorsIndexes(ctx)
- if err != nil {
-     log.WithError(err).Warn("failed to get connectors index")
-     return
- }
+ connectorsIndex, err := s.cache.ForfeitTxs().GetConnectorsIndexes(ctx)
+ if err != nil {
+     log.WithError(err).Warn("failed to get connectors index, propagating without connector topics")
+     connectorsIndex = make(map[string]domain.Outpoint)
+ }

This matches the earlier review suggestion and avoids silently dropping RoundFinalizationStarted propagation when Redis is flaky.

internal/core/application/service.go (2)

2271-2292: Guard logging on existingRound to avoid relying on non‑nil contract

Here you read existingRound.Id unconditionally for logging:

existingRound, err := s.cache.CurrentRound().Get(ctx)
...
if err := s.cache.ForfeitTxs().Reset(ctx); err != nil {
    log.WithError(err).Warnf(
        "failed to delete forfeit txs from cache for round %s", existingRound.Id,
    )
}
...
if err := s.cache.Intents().DeleteVtxos(ctx); err != nil {
    log.WithError(err).Warnf(
        "failed to delete spent vtxos from cache after round %s", existingRound.Id,
    )
}
...
if err := s.cache.ConfirmationSessions().Reset(ctx); err != nil {
    log.WithError(err).Errorf(
        "failed to reset confirmation session from cache for round %s", existingRound.Id,
    )
}

This assumes CurrentRound().Get will never return (*domain.Round)(nil) when err == nil. If that contract is ever broken (e.g., a “not found” mapped to (nil, nil)), these logs will panic even though you’re just trying to reset caches.

Safer pattern:

 existingRound, err := s.cache.CurrentRound().Get(ctx)
 ...
- if err := s.cache.ForfeitTxs().Reset(ctx); err != nil {
-     log.WithError(err).Warnf(
-         "failed to delete forfeit txs from cache for round %s", existingRound.Id,
-     )
- }
+ roundID := "<none>"
+ if existingRound != nil {
+     roundID = existingRound.Id
+ }
+
+ if err := s.cache.ForfeitTxs().Reset(ctx); err != nil {
+     log.WithError(err).Warnf(
+         "failed to delete forfeit txs from cache for round %s", roundID,
+     )
+ }
  ...
- if err := s.cache.Intents().DeleteVtxos(ctx); err != nil {
+ if err := s.cache.Intents().DeleteVtxos(ctx); err != nil {
      log.WithError(err).Warnf(
-         "failed to delete spent vtxos from cache after round %s", existingRound.Id,
+         "failed to delete spent vtxos from cache after round %s", roundID,
      )
  }
  if err := s.cache.ConfirmationSessions().Reset(ctx); err != nil {
      log.WithError(err).Errorf(
-         "failed to reset confirmation session from cache for round %s", existingRound.Id,
+         "failed to reset confirmation session from cache for round %s", roundID,
      )
  }

This keeps behavior the same when a round exists, but protects against future changes in the cache implementation.

---

3454-3479: Consider using service context instead of context.Background() in forfeits/boarding watcher

checkForfeitsAndBoardingSigsSent currently hard‑codes context.Background():

func (s *service) checkForfeitsAndBoardingSigsSent(commitmentTxid string) {
    ctx := context.Background()
    ...
    sigs, err := s.cache.BoardingInputs().GetSignatures(ctx, commitmentTxid)
    ...
    numOfBoardingInputs, err := s.cache.BoardingInputs().Get(ctx)
    ...
    allForfeitTxsSigned, err := s.cache.ForfeitTxs().AllSigned(ctx)

Since this function is tied to a specific round and is invoked from SubmitForfeitTxs / SignCommitmentTx, it arguably should respect the service lifecycle. Using s.ctx (or receiving a ctx parameter) would allow Stop() to cancel any long‑running Redis calls rather than leaving them uncancellable.

Not a correctness bug, but you may want to align with the rest of the round pipeline which already gates on s.ctx.Done().

Based on learnings, this is about service‑lifetime cancellation, not request‑lifetime, so it shouldn’t conflict with the intentional context.Background() usage in reactToFraud.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: louisinger
Repo: arkade-os/arkd PR: 686
File: internal/core/application/fraud.go:47-61
Timestamp: 2025-08-28T08:21:01.170Z
Learning: In reactToFraud function in internal/core/application/fraud.go, the goroutine that waits for confirmation and schedules checkpoint sweep should use context.Background() instead of the request context, as this is intentional design to decouple the checkpoint sweep scheduling from the request lifetime.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 686
File: internal/core/application/fraud.go:47-61
Timestamp: 2025-08-28T08:21:01.170Z
Learning: In reactToFraud function in internal/core/application/fraud.go, the goroutine that waits for confirmation and schedules checkpoint sweep should use context.Background() instead of the request context, as this is intentional design to decouple the checkpoint sweep scheduling from the request lifetime.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.

internal/core/application/service.go (2)

3334-3338: [Duplicate] Error in connector index fetch silently aborts event propagation.

If GetConnectorsIndexes fails, the function returns early without propagating any events. This leaves clients without notification of the round finalization.

As noted in previous review, consider logging the error and continuing with an empty connector topics map, or propagating a partial event:

 	connectorsIndex, err := s.cache.ForfeitTxs().GetConnectorsIndexes(ctx)
 	if err != nil {
 		log.WithError(err).Warn("failed to get connectors index")
-		return
+		connectorsIndex = make(map[string]domain.Outpoint)
 	}

---

3348-3352: [Duplicate] Error fetching selected intents prevents RoundFailed propagation.

If GetSelectedIntents fails, the RoundFailed event is not propagated to clients. This is critical because clients need to know the round failed to handle their pending intents appropriately.

As noted in previous review, consider propagating the failure event with empty topics rather than silently returning:

 case domain.RoundFailed:
 	intents, err := s.cache.Intents().GetSelectedIntents(ctx)
 	if err != nil {
 		log.WithError(err).Warn("failed to get selected intents")
-		return
+		intents = []ports.TimedIntent{}
 	}

internal/infrastructure/live-store/inmemory/boarding_inputs.go (1)

38-54: batchId parameter is still ignored across all signature methods.

The batchId parameter is ignored in AddSignatures, GetSignatures, and DeleteSignatures. This was previously flagged — signatures from different batches will be mixed together, which may cause incorrect behavior if multiple batches are processed.

Also applies to: 64-68

internal/infrastructure/live-store/redis/confirmation_sessions.go (1)

94-130: Race condition: numConfirmed is still read outside the transaction.

The numConfirmed value is read at line 111, but used inside the Watch callback at line 119. If another process confirms an intent between the read and the transaction, the count will be incorrect. The Watch call at line 116 should also watch confirmationNumConfirmedKey, and the read should be inside the callback.

internal/infrastructure/live-store/redis/forfeits.go (1)

176-193: Reset retry delay issue from earlier review is resolved.

Reset now uses the same for range s.numOfRetries + Watch + TxPipelined pattern as other methods and includes time.Sleep(s.retryDelay) between attempts, preventing tight-loop retries.

internal/infrastructure/live-store/redis/round.go (2)

52-64: Minor inefficiency: unnecessary sleep after final retry.

The retry loop sleeps after every attempt, including the final failed one. The last sleep is wasteful since the function returns immediately afterward.

Consider tracking the iteration to skip the sleep after the last attempt:

-	for range s.numOfRetries {
+	for i := range s.numOfRetries {
 		if err = s.rdb.Watch(ctx, func(tx *redis.Tx) error {
 			_, err = tx.TxPipelined(ctx, func(pipe redis.Pipeliner) error {
 				pipe.Set(ctx, currentRoundKey, val, 0)
 				return nil
 			})
 
 			return err
 		}); err == nil {
 			return nil
 		}
-		time.Sleep(s.retryDelay)
+		if i < s.numOfRetries-1 {
+			time.Sleep(s.retryDelay)
+		}
 	}

---

163-165: Consider whether silently skipping unknown event types is intended.

The default case at line 164 uses continue to silently skip unknown event types. This provides forward compatibility (older code can ignore new event types), but it could also hide bugs if an expected event type is missing due to a typo or version mismatch.

If this silent skip is intentional for backward compatibility, consider adding a log statement or comment documenting the design decision. If not, consider returning an error for truly unknown types.

internal/core/application/service.go (1)

3213-3218: Verify VtxoTree is not nil before accessing.

After round.EndFinalization() completes, the code accesses round.VtxoTree.Leaves() and len(round.VtxoTree) without checking if VtxoTree is nil. While EndFinalization likely populates this field, if the round had no vtxo tree (e.g., only boarding inputs), this could panic.

Consider adding a nil check for defensive programming:

+	totalOutputVtxos := 0
+	numOfTreeNodes := 0
+	if round.VtxoTree != nil {
+		totalOutputVtxos = len(round.VtxoTree.Leaves())
+		numOfTreeNodes = len(round.VtxoTree)
+	}
-	totalOutputVtxos := len(round.VtxoTree.Leaves())
-	numOfTreeNodes := len(round.VtxoTree)

internal/infrastructure/live-store/redis/boarding_inputs.go (2)

50-56: Consider handling redis.Nil case explicitly in Get.

When the key doesn't exist, Get returns -1 with a redis.Nil error. Callers may need to distinguish between "not initialized" (key doesn't exist) and actual errors. The in-memory version returns 0 by default when not set.

 func (b *boardingInputsStore) Get(ctx context.Context) (int, error) {
 	num, err := b.rdb.Get(ctx, boardingInputsKey).Int()
 	if err != nil {
+		if errors.Is(err, redis.Nil) {
+			return 0, nil
+		}
 		return -1, err
 	}
 	return num, nil
 }

You'll need to add "errors" to the imports.

---

122-125: Consider adding retry logic for consistency.

DeleteSignatures doesn't use retry logic, unlike other write operations (Set, AddSignatures). While delete operations are generally idempotent, this creates inconsistency. If transient Redis errors are a concern for other operations, they should be for this one too.

internal/infrastructure/live-store/inmemory/forfeits.go (1)

126-132: Error from Reset is silently ignored.

The // nolint comment indicates that the error return from Reset(ctx) is intentionally ignored. While Reset currently always returns nil in the in-memory implementation, this pattern could mask issues if the implementation changes or if this pattern is copied to other stores.

Consider logging or handling the error:

 	defer func() {
 		m.lock.Unlock()
-		// nolint
-		m.Reset(ctx)
+		if err := m.Reset(ctx); err != nil {
+			// Log but don't fail - best effort cleanup
+			log.Warnf("failed to reset forfeit store after pop: %v", err)
+		}
 	}()

internal/infrastructure/live-store/redis/confirmation_sessions.go (1)

132-156: Get reads multiple keys non-atomically.

The Get method reads confirmationIntentsKey, confirmationNumIntentsKey, and confirmationNumConfirmedKey in separate Redis calls. If another process modifies the state between reads, the returned ConfirmationSessions could be inconsistent (e.g., NumConfirmedIntents doesn't match the count of confirmed entries in IntentsHashes).

Consider using a Lua script or transaction for atomic reads if consistency is important.

internal/infrastructure/live-store/redis/tree_signing_session.go (1)

93-153: Get performs multiple non-atomic Redis reads.

The method reads metaKey, noncesKey, and sigsKey in separate HGetAll calls. If another process modifies the session between reads, the returned MusigSigningSession could be inconsistent.

For read operations, consider using a Lua script or accepting eventual consistency if that's acceptable for this use case.

internal/infrastructure/live-store/redis/forfeits.go (1)

196-225: Pop semantics and error handling look good.

Pop:

• Validates that each stored forfeit has JSON and that connectors aren’t reused.
• Collects the signed txs and then calls Reset(ctx) to clear state, propagating any reset error.

Only (optional) improvement would be to wrap the initial HGetAll error with context (e.g., "failed to get forfeit txs: %v"), but behavior is functionally correct.

internal/infrastructure/live-store/redis/intents.go (1)

58-123: Retry logic should differentiate transient TxFailedErr from deterministic validation errors.

The suggestion to separate retry-worthy transient failures from application logic errors is sound. In go-redis/v9, the sentinel TxFailedErr is returned when EXEC returns no results because WATCHed keys were modified, and it's recommended to treat it as a special, expected/retryable condition rather than an application logic error.

Currently, the code retries on any error without differentiation, which means deterministic validation errors (like "duplicated intent %s" or "duplicated input...") waste retry iterations. Instead, use errors.Is(err, redis.TxFailedErr) to detect the transaction failure and retry, while returning other errors normally.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: louisinger
Repo: arkade-os/arkd PR: 722
File: pkg/ark-lib/intent/proof.go:52-58
Timestamp: 2025-09-29T14:33:52.871Z
Learning: In btcsuite/btcd's psbt package, the NewFromRawBytes function's boolean parameter (b64) automatically handles base64 decoding when set to true, so passing a base64 string via strings.NewReader with b64=true is the correct usage pattern. A common bug is manually base64-decoding the string and then passing b64=true, which causes a double-decode error.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 722
File: pkg/ark-lib/intent/proof.go:52-58
Timestamp: 2025-09-29T14:33:52.871Z
Learning: In btcsuite/btcd's psbt package, the NewFromRawBytes function's boolean parameter (b64) automatically handles base64 decoding when set to true, so passing a base64 string via strings.NewReader with b64=true is the correct usage pattern.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 686
File: internal/core/application/fraud.go:47-61
Timestamp: 2025-08-28T08:21:01.170Z
Learning: In reactToFraud function in internal/core/application/fraud.go, the goroutine that waits for confirmation and schedules checkpoint sweep should use context.Background() instead of the request context, as this is intentional design to decouple the checkpoint sweep scheduling from the request lifetime.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.

internal/infrastructure/live-store/redis/intents.go (1)

136-185: Handle nil intents in Pop and fix remaining SAdd slice bug for vtxosToRemove.

Two issues in Pop/DeleteVtxos:

1. Possible nil dereference in Pop:

intent, err := s.intents.Get(ctx, id)
if err != nil {
    return nil, fmt.Errorf("failed to get intent %s: %v", id, err)
}
if len(intent.Receivers) > 0 {  // intent may be nil here
    intentsByTime = append(intentsByTime, *intent)
}

If the KV store returns (nil, nil) for a missing intent, this will panic. Add a nil check:

 	intent, err := s.intents.Get(ctx, id)
 	if err != nil {
 		return nil, fmt.Errorf("failed to get intent %s: %v", id, err)
 	}
-
-	if len(intent.Receivers) > 0 {
+	if intent != nil && len(intent.Receivers) > 0 {
 		intentsByTime = append(intentsByTime, *intent)
 	}

2. SAdd still called with []string instead of variadic members (previously flagged):

if len(vtxosToRemove) > 0 {
    s.rdb.SAdd(ctx, intentStoreVtxosToRemoveKey, vtxosToRemove)
}

Passing a []string without conversion/... sends the slice as a single member; Redis will not see individual vtxo strings, and DeleteVtxos will later remove the wrong thing. A prior review already called out this exact issue for Pop and DeleteVtxos; you’ve fixed DeleteVtxos (now converts to []interface{} and uses members...), but Pop still has the old pattern.

Mirror the DeleteVtxos fix here:

-	if len(vtxosToRemove) > 0 {
-		s.rdb.SAdd(ctx, intentStoreVtxosToRemoveKey, vtxosToRemove)
-	}
+	if len(vtxosToRemove) > 0 {
+		members := make([]interface{}, len(vtxosToRemove))
+		for i, v := range vtxosToRemove {
+			members[i] = v
+		}
+		if err := s.rdb.SAdd(ctx, intentStoreVtxosToRemoveKey, members...).Err(); err != nil {
+			return nil, fmt.Errorf("failed to enqueue vtxos to remove: %v", err)
+		}
+	}

With this, the vtxosToRemove set and DeleteVtxos’s SRem logic will be consistent and actually clear VTXOs from intentStoreVtxosKey.

Also applies to: 326-352

internal/infrastructure/live-store/redis/confirmation_sessions.go (1)

113-134: Move numConfirmed read inside the WATCH transaction to avoid lost updates.

numConfirmed is read once before the retry loop:

numConfirmed, err := s.rdb.Get(ctx, confirmationNumConfirmedKey).Int()

and then reused inside the Watch callback without re-reading under the WATCHed transaction. If another process confirms a different intent between this read and a successful transaction, its increment can be lost (the later transaction will overwrite confirmationNumConfirmedKey with staleNumConfirmed+1).

This is the same race that was already pointed out in a previous review. You can fix it by reading numConfirmed inside the WATCH callback:

 func (s *confirmationSessionsStore) Confirm(ctx context.Context, intentId string) error {
 	// ... existing confirmed check ...

-	numConfirmed, err := s.rdb.Get(ctx, confirmationNumConfirmedKey).Int()
-	if err != nil && !errors.Is(err, redis.Nil) {
-		return fmt.Errorf("failed to get number of confirmed intents: %v", err)
-	}
-
 	keys := []string{confirmationIntentsKey, confirmationNumConfirmedKey}
-	for range s.numOfRetries {
-		if err = s.rdb.Watch(ctx, func(tx *redis.Tx) error {
-			_, err := tx.TxPipelined(ctx, func(pipe redis.Pipeliner) error {
-				pipe.HSet(ctx, confirmationIntentsKey, hashKey, 1)
-				pipe.Set(ctx, confirmationNumConfirmedKey, numConfirmed+1, 0)
-				return nil
-			})
-			return err
-		}, keys...); err == nil {
-			return nil
-		}
-		time.Sleep(s.retryDelay)
-	}
+	for i := 0; i < s.numOfRetries; i++ {
+		if err = s.rdb.Watch(ctx, func(tx *redis.Tx) error {
+			numConfirmed, err := tx.Get(ctx, confirmationNumConfirmedKey).Int()
+			if err != nil && !errors.Is(err, redis.Nil) {
+				return fmt.Errorf("failed to get number of confirmed intents: %v", err)
+			}
+			_, err = tx.TxPipelined(ctx, func(pipe redis.Pipeliner) error {
+				pipe.HSet(ctx, confirmationIntentsKey, hashKey, 1)
+				pipe.Set(ctx, confirmationNumConfirmedKey, numConfirmed+1, 0)
+				return nil
+			})
+			return err
+		}, keys...); err == nil {
+			return nil
+		}
+		if i < s.numOfRetries-1 {
+			time.Sleep(s.retryDelay)
+		}
+	}

This way each confirm observes the latest count under WATCH and increments it without losing concurrent confirmations.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: louisinger
Repo: arkade-os/arkd PR: 686
File: internal/core/application/fraud.go:47-61
Timestamp: 2025-08-28T08:21:01.170Z
Learning: In reactToFraud function in internal/core/application/fraud.go, the goroutine that waits for confirmation and schedules checkpoint sweep should use context.Background() instead of the request context, as this is intentional design to decouple the checkpoint sweep scheduling from the request lifetime.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 722
File: pkg/ark-lib/intent/proof.go:52-58
Timestamp: 2025-09-29T14:33:52.871Z
Learning: In btcsuite/btcd's psbt package, the NewFromRawBytes function's boolean parameter (b64) automatically handles base64 decoding when set to true, so passing a base64 string via strings.NewReader with b64=true is the correct usage pattern. A common bug is manually base64-decoding the string and then passing b64=true, which causes a double-decode error.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 722
File: pkg/ark-lib/intent/proof.go:52-58
Timestamp: 2025-09-29T14:33:52.871Z
Learning: In btcsuite/btcd's psbt package, the NewFromRawBytes function's boolean parameter (b64) automatically handles base64 decoding when set to true, so passing a base64 string via strings.NewReader with b64=true is the correct usage pattern.

internal/infrastructure/live-store/inmemory/forfeits.go (1)

117-133: Fix deadlock between Pop and Reset and avoid calling a locking method while holding the lock

Pop acquires m.lock and defers a call to m.Reset(ctx), but Reset also acquires m.lock. Since sync.Mutex/RWMutex are not reentrant, this will deadlock. This is the same issue previously flagged in an earlier review.

Refactor the reset logic into a private helper that assumes the lock is held, use it from both Reset and Pop, and avoid calling Reset from inside the locked region:

-func (m *forfeitTxsStore) Reset(_ context.Context) error {
-	m.lock.Lock()
-	defer m.lock.Unlock()
-
-	m.forfeitTxs = make(map[domain.Outpoint]ports.ValidForfeitTx)
-	m.connectors = nil
-	m.connectorsIndex = nil
-	m.vtxos = nil
-	return nil
-}
-func (m *forfeitTxsStore) Pop(ctx context.Context) ([]string, error) {
-	m.lock.Lock()
-	defer func() {
-		m.lock.Unlock()
-		// nolint
-		m.Reset(ctx)
-	}()
+func (m *forfeitTxsStore) resetLocked() {
+	m.forfeitTxs = make(map[domain.Outpoint]ports.ValidForfeitTx)
+	m.connectors = nil
+	m.connectorsIndex = nil
+	m.vtxos = nil
+}
+
+func (m *forfeitTxsStore) Reset(_ context.Context) error {
+	m.lock.Lock()
+	defer m.lock.Unlock()
+
+	m.resetLocked()
+	return nil
+}
+
+func (m *forfeitTxsStore) Pop(_ context.Context) ([]string, error) {
+	m.lock.Lock()
+	defer m.lock.Unlock()
@@
-	return txs, nil
+	m.resetLocked()
+	return txs, nil
 }

This keeps the reset atomic with the Pop operation, removes the need for // nolint, and avoids the deadlock.

internal/infrastructure/live-store/redis/boarding_inputs.go (4)

50-56: Consider handling redis.Nil explicitly for unset keys.

When the key doesn't exist, redis.Nil is returned as an error. Callers may need to distinguish between "key not set" and "actual error". Consider returning a sentinel value or wrapping the error for clarity.

 func (b *boardingInputsStore) Get(ctx context.Context) (int, error) {
 	num, err := b.rdb.Get(ctx, boardingInputsKey).Int()
 	if err != nil {
+		if err == redis.Nil {
+			return 0, nil // Key not set, return default
+		}
 		return -1, err
 	}
 	return num, nil
 }

---

79-95: Wrap final error with context for consistency.

Unlike the Set method which wraps the error with a descriptive message, AddSignatures returns the raw error after exhausting retries. This makes debugging harder.

 	}
-	return err
+	return fmt.Errorf(
+		"failed to add boarding input signatures after max number of retries: %v", err,
+	)
 }

---

100-118: Variable shadowing: key is reused in the loop.

The outer key (Redis key) is shadowed by the loop variable key (hash field name). While the code works correctly, this is confusing and a code smell.

 func (b *boardingInputsStore) GetSignatures(
 	ctx context.Context, batchId string,
 ) (map[uint32]ports.SignedBoardingInput, error) {
 	key := fmt.Sprintf("%s:%s", boardingInputSigsKey, batchId)
 	values, err := b.rdb.HGetAll(ctx, key).Result()
 	if err != nil {
 		return nil, err
 	}
 	m := make(map[uint32]ports.SignedBoardingInput)
-	for key, value := range values {
+	for field, value := range values {
 		rawSig := &sigsDTO{}
 		sig, err := rawSig.deserialize([]byte(value))
 		if err != nil {
-			return nil, fmt.Errorf("malformed signatures for input %s in storage: %v", key, err)
+			return nil, fmt.Errorf("malformed signatures for input %s in storage: %v", field, err)
 		}
-		inIndex, err := strconv.Atoi(key)
+		inIndex, err := strconv.Atoi(field)
 		if err != nil {
 			return nil, err
 		}

 		m[uint32(inIndex)] = *sig
 	}
 	return m, nil
 }

---

170-173: Value receiver on deserialize is unconventional.

The method has a value receiver but immediately unmarshals into &s. While this works, it's confusing. Consider using a pointer receiver or a standalone function for clarity.

-func (s sigsDTO) deserialize(buf []byte) (*ports.SignedBoardingInput, error) {
-	if err := json.Unmarshal(buf, &s); err != nil {
+func (s *sigsDTO) deserialize(buf []byte) (*ports.SignedBoardingInput, error) {
+	if err := json.Unmarshal(buf, s); err != nil {
 		return nil, err
 	}

internal/infrastructure/live-store/inmemory/intents.go (1)

102-136: LGTM: Context and error return added.

The method signature correctly implements the updated interface. The sorting and selection logic remains sound.

For very large intent pools, consider checking ctx.Err() before/after the sort operation to allow early cancellation. However, since the operation holds a write lock, completing atomically may be the correct design choice.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: louisinger
Repo: arkade-os/arkd PR: 686
File: internal/core/application/fraud.go:47-61
Timestamp: 2025-08-28T08:21:01.170Z
Learning: In reactToFraud function in internal/core/application/fraud.go, the goroutine that waits for confirmation and schedules checkpoint sweep should use context.Background() instead of the request context, as this is intentional design to decouple the checkpoint sweep scheduling from the request lifetime.

internal/core/application/service.go (3)

2876-2911: Error ignored from Get could cause nil pointer dereference.

At line 2877, the error from s.cache.TreeSigingSessions().Get(ctx, roundId) is ignored. If signingSession is nil, iterating over signingSession.Signatures at line 2880 would panic. This was flagged in a previous review.

Apply this diff:

 case <-s.cache.TreeSigingSessions().SignaturesCollected(roundId):
-	signingSession, _ := s.cache.TreeSigingSessions().Get(ctx, roundId)
+	signingSession, err := s.cache.TreeSigingSessions().Get(ctx, roundId)
+	if err != nil || signingSession == nil {
+		round.Fail(errors.INTERNAL_ERROR.New("failed to get signing session from cache"))
+		return
+	}
 	cosignersToBan := make(map[string]domain.Crime)

---

3337-3355: Error handling still prevents event propagation.

As flagged in previous reviews:

1. Lines 3337-3341: If GetConnectorsIndexes fails, returning early prevents event propagation
2. Lines 3351-3355: If GetSelectedIntents fails, RoundFailed event is not propagated

These were previously suggested to continue with empty values rather than returning early.

---

2514-2521: roundAborted should be set to true on confirmation session retrieval failure.

When s.cache.ConfirmationSessions().Get(ctx) fails, the code calls round.Fail() and returns, but roundAborted remains false. This causes the defer to attempt saving events and check round.IsFailed() instead of immediately restarting the round via the roundAborted path.

This was flagged in a previous review but the fix appears incomplete.

Apply this diff:

 case <-time.After(roundTiming.confirmationDuration()):
 	session, err := s.cache.ConfirmationSessions().Get(ctx)
 	if err != nil {
+		roundAborted = true
 		log.WithError(err).Error("failed to get confirmation session from cache")
 		round.Fail(errors.INTERNAL_ERROR.New(
 			"failed to get confirmation session from cache: %s", err,
 		))
 		return
 	}

internal/infrastructure/live-store/redis/boarding_inputs.go (2)

23-29: Consider making retry delay configurable.

The retry delay is hardcoded to 10ms. While this appears consistent across the codebase, making it a constructor parameter would improve flexibility for different deployment scenarios or testing.

-func NewBoardingInputsStore(rdb *redis.Client, numOfRetries int) ports.BoardingInputsStore {
+func NewBoardingInputsStore(rdb *redis.Client, numOfRetries int, retryDelay time.Duration) ports.BoardingInputsStore {
 	return &boardingInputsStore{
 		rdb:          rdb,
 		numOfRetries: numOfRetries,
-		retryDelay:   10 * time.Millisecond,
+		retryDelay:   retryDelay,
 	}
 }

---

50-56: Consider returning zero value instead of -1 on error.

Returning -1 as a sentinel value is unconventional in Go. The idiomatic approach is to return the zero value (0) and rely on the error to indicate failure.

 func (b *boardingInputsStore) Get(ctx context.Context) (int, error) {
 	num, err := b.rdb.Get(ctx, boardingInputsKey).Int()
 	if err != nil {
-		return -1, err
+		return 0, err
 	}
 	return num, nil
 }

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: louisinger
Repo: arkade-os/arkd PR: 686
File: internal/core/application/fraud.go:47-61
Timestamp: 2025-08-28T08:21:01.170Z
Learning: In reactToFraud function in internal/core/application/fraud.go, the goroutine that waits for confirmation and schedules checkpoint sweep should use context.Background() instead of the request context, as this is intentional design to decouple the checkpoint sweep scheduling from the request lifetime.

Learnt from: louisinger
Repo: arkade-os/arkd PR: 691
File: internal/core/application/service.go:557-562
Timestamp: 2025-08-19T10:58:41.042Z
Learning: In the arkd SubmitOffchainTx method, using the checkpoint PSBT input's tapscript (forfeit path) for the VtxoInput.Tapscript field is the correct behavior, not a bug as initially thought. The system correctly handles the relationship between checkpoint inputs and Ark transaction inputs.