Fix RBF until events arrive

[ben-kaufman]

Fix RBF until onchain events arrive

Screen.Recording.2025-11-27.at.11.18.08.AM.mov

[claude]

Pull Request Review: Fix RBF until events arrive

Summary

This PR addresses a timing issue in RBF (Replace-By-Fee) transaction handling. The change ensures that the original transaction remains visible in the UI with a boosting status until the replacement transaction is detected via onchain events, rather than immediately marking it as removed.

Code Quality and Architecture

Positive aspects:

• Clean fix: The solution is minimal and focused, changing only what's necessary
• Symmetric handling: CPFP already used isBoosted = true with boostTxIds.append(), and now RBF uses the same pattern temporarily
• Proper cleanup: When the replacement event arrives, both doesExist = false and isBoosted = false are set (CoreService.swift:359)
• Good logging: Clear log messages explain the state transitions

Changes align with AGENTS.md conventions:

• Uses proper service layer patterns
• Maintains existing Bitcoin/Lightning terminology
• Follows the established activity tracking patterns

Logic Analysis

Before this PR:
In boostOnchainTransaction (RBF path), the code set onchainActivity.doesExist = false immediately

After this PR:
In boostOnchainTransaction (RBF path), the code sets onchainActivity.isBoosted = true (NOT adding to boostTxIds here - that happens in handleOnchainTransactionReplaced)

Why this works:

1. User initiates RBF and original tx is marked with isBoosted = true
2. UI shows Confirming in ~X min (boosted) status (ActivityItemView.swift:277-284)
3. LDK fires onchainTransactionReplaced event
4. Handler marks original as doesExist = false AND isBoosted = false (CoreService.swift:358-359)
5. Replacement tx gets isBoosted = true and boostTxIds updated (CoreService.swift:404-405)

Potential Issues and Questions

1. Inconsistency with CPFP pattern
In CPFP (line 986-987), the code sets both isBoosted = true and appends to boostTxIds. In RBF (line 1003), it only sets isBoosted = true without adding to boostTxIds.

This appears intentional since CPFP parent tx still exists (so it makes sense to add the child txid immediately) while RBF original tx will be replaced (and the replacement gets tracked via the event handler). However, a clarifying comment would be helpful.

2. Race condition consideration
What happens if the onchainTransactionReplaced event arrives before the boostOnchainTransaction update completes? Both operations use ServiceQueue.background(.core), so they should be serialized. This is likely safe, but worth verifying in testing.

3. ActivityItemViewModel refresh logic
In ActivityItemViewModel.swift:77-90, there is logic that searches for RBF replacements by matching isBoosted = true, same txType, same value, and later timestamp. During the window where the original has isBoosted = true but the replacement has not arrived yet, this logic might incorrectly match the original to itself. The 3-attempt retry logic (line 95-99) suggests this was anticipated, which is good.

Performance Considerations

Positive:

• No additional database queries
• No blocking operations added
• The removed log statement (AppViewModel.swift) reduces noise

Suggestion: The removed log was actually useful for debugging. Consider downgrading to .debug level instead of removing it entirely.

Security and Bitcoin Considerations

Secure:

• No changes to actual RBF transaction creation
• No changes to fee calculation
• Proper state transitions maintain data integrity
• Users get better feedback (transaction stays visible during boost)

Test Coverage

Existing tests:

• TxBumpingTests.swift has comprehensive RBF testing (testBumpFeeByRbf)
• Tests verify the RBF transaction is created and confirmed
• Gap: Tests do not verify the isBoosted flag behavior or UI state transitions

Recommendation: Add a unit test to verify the isBoosted state transitions during RBF

UI/UX Impact

User experience improvement:

• Users now see Confirming in ~X min (boosted) instead of transaction disappearing
• Reduces confusion during the event propagation window
• Status icon changes to hourglass with yellow accent

Edge case to verify: If the replacement event never arrives (network issue, node restart, etc.), the original stays in boosted state indefinitely. Is there a timeout or recovery mechanism?

Recommendations

1. Add clarifying comment (optional but helpful) at CoreService.swift:1003
2. Restore debug logging (optional) in AppViewModel.swift
3. Add unit test for isBoosted state transitions during RBF (recommended)
4. Manual testing of the edge cases mentioned above

Verdict: LGTM with minor suggestions

This is a well-targeted fix that improves UX during RBF operations. The code is clean, follows existing patterns, and addresses a real timing issue. The suggestions above are mostly nice-to-haves for clarity and testing robustness.

Approve with optional follow-ups. The PR is ready to merge as-is, with the recommendations being good candidates for future improvements.

[ben-kaufman]

Fix RBF until onchain events arrive

Screen.Recording.2025-11-27.at.11.18.08.AM.mov

[claude]

Pull Request Review: Fix RBF until events arrive

Summary

This PR addresses a timing issue in RBF (Replace-By-Fee) transaction handling. The change ensures that the original transaction remains visible in the UI with a boosting status until the replacement transaction is detected via onchain events, rather than immediately marking it as removed.

Code Quality and Architecture

Positive aspects:

• Clean fix: The solution is minimal and focused, changing only what's necessary
• Symmetric handling: CPFP already used isBoosted = true with boostTxIds.append(), and now RBF uses the same pattern temporarily
• Proper cleanup: When the replacement event arrives, both doesExist = false and isBoosted = false are set (CoreService.swift:359)
• Good logging: Clear log messages explain the state transitions

Changes align with AGENTS.md conventions:

• Uses proper service layer patterns
• Maintains existing Bitcoin/Lightning terminology
• Follows the established activity tracking patterns

Logic Analysis

Before this PR:
In boostOnchainTransaction (RBF path), the code set onchainActivity.doesExist = false immediately

After this PR:
In boostOnchainTransaction (RBF path), the code sets onchainActivity.isBoosted = true (NOT adding to boostTxIds here - that happens in handleOnchainTransactionReplaced)

Why this works:

1. User initiates RBF and original tx is marked with isBoosted = true
2. UI shows Confirming in ~X min (boosted) status (ActivityItemView.swift:277-284)
3. LDK fires onchainTransactionReplaced event
4. Handler marks original as doesExist = false AND isBoosted = false (CoreService.swift:358-359)
5. Replacement tx gets isBoosted = true and boostTxIds updated (CoreService.swift:404-405)

Potential Issues and Questions

1. Inconsistency with CPFP pattern
In CPFP (line 986-987), the code sets both isBoosted = true and appends to boostTxIds. In RBF (line 1003), it only sets isBoosted = true without adding to boostTxIds.

This appears intentional since CPFP parent tx still exists (so it makes sense to add the child txid immediately) while RBF original tx will be replaced (and the replacement gets tracked via the event handler). However, a clarifying comment would be helpful.

2. Race condition consideration
What happens if the onchainTransactionReplaced event arrives before the boostOnchainTransaction update completes? Both operations use ServiceQueue.background(.core), so they should be serialized. This is likely safe, but worth verifying in testing.

3. ActivityItemViewModel refresh logic
In ActivityItemViewModel.swift:77-90, there is logic that searches for RBF replacements by matching isBoosted = true, same txType, same value, and later timestamp. During the window where the original has isBoosted = true but the replacement has not arrived yet, this logic might incorrectly match the original to itself. The 3-attempt retry logic (line 95-99) suggests this was anticipated, which is good.

Performance Considerations

Positive:

• No additional database queries
• No blocking operations added
• The removed log statement (AppViewModel.swift) reduces noise

Suggestion: The removed log was actually useful for debugging. Consider downgrading to .debug level instead of removing it entirely.

Security and Bitcoin Considerations

Secure:

• No changes to actual RBF transaction creation
• No changes to fee calculation
• Proper state transitions maintain data integrity
• Users get better feedback (transaction stays visible during boost)

Test Coverage

Existing tests:

• TxBumpingTests.swift has comprehensive RBF testing (testBumpFeeByRbf)
• Tests verify the RBF transaction is created and confirmed
• Gap: Tests do not verify the isBoosted flag behavior or UI state transitions

Recommendation: Add a unit test to verify the isBoosted state transitions during RBF

UI/UX Impact

User experience improvement:

• Users now see Confirming in ~X min (boosted) instead of transaction disappearing
• Reduces confusion during the event propagation window
• Status icon changes to hourglass with yellow accent

Edge case to verify: If the replacement event never arrives (network issue, node restart, etc.), the original stays in boosted state indefinitely. Is there a timeout or recovery mechanism?

Recommendations

1. Add clarifying comment (optional but helpful) at CoreService.swift:1003
2. Restore debug logging (optional) in AppViewModel.swift
3. Add unit test for isBoosted state transitions during RBF (recommended)
4. Manual testing of the edge cases mentioned above

Verdict: LGTM with minor suggestions

This is a well-targeted fix that improves UX during RBF operations. The code is clean, follows existing patterns, and addresses a real timing issue. The suggestions above are mostly nice-to-haves for clarity and testing robustness.

Approve with optional follow-ups. The PR is ready to merge as-is, with the recommendations being good candidates for future improvements.

[ben-kaufman]

Fix RBF until onchain events arrive

Screen.Recording.2025-11-27.at.11.18.08.AM.mov

[claude]

Pull Request Review: Fix RBF until events arrive

Summary

This PR addresses a timing issue in RBF (Replace-By-Fee) transaction handling. The change ensures that the original transaction remains visible in the UI with a boosting status until the replacement transaction is detected via onchain events, rather than immediately marking it as removed.

Code Quality and Architecture

Positive aspects:

• Clean fix: The solution is minimal and focused, changing only what's necessary
• Symmetric handling: CPFP already used isBoosted = true with boostTxIds.append(), and now RBF uses the same pattern temporarily
• Proper cleanup: When the replacement event arrives, both doesExist = false and isBoosted = false are set (CoreService.swift:359)
• Good logging: Clear log messages explain the state transitions

Changes align with AGENTS.md conventions:

• Uses proper service layer patterns
• Maintains existing Bitcoin/Lightning terminology
• Follows the established activity tracking patterns

Logic Analysis

Before this PR:
In boostOnchainTransaction (RBF path), the code set onchainActivity.doesExist = false immediately

After this PR:
In boostOnchainTransaction (RBF path), the code sets onchainActivity.isBoosted = true (NOT adding to boostTxIds here - that happens in handleOnchainTransactionReplaced)

Why this works:

1. User initiates RBF and original tx is marked with isBoosted = true
2. UI shows Confirming in ~X min (boosted) status (ActivityItemView.swift:277-284)
3. LDK fires onchainTransactionReplaced event
4. Handler marks original as doesExist = false AND isBoosted = false (CoreService.swift:358-359)
5. Replacement tx gets isBoosted = true and boostTxIds updated (CoreService.swift:404-405)

Potential Issues and Questions

1. Inconsistency with CPFP pattern
In CPFP (line 986-987), the code sets both isBoosted = true and appends to boostTxIds. In RBF (line 1003), it only sets isBoosted = true without adding to boostTxIds.

This appears intentional since CPFP parent tx still exists (so it makes sense to add the child txid immediately) while RBF original tx will be replaced (and the replacement gets tracked via the event handler). However, a clarifying comment would be helpful.

2. Race condition consideration
What happens if the onchainTransactionReplaced event arrives before the boostOnchainTransaction update completes? Both operations use ServiceQueue.background(.core), so they should be serialized. This is likely safe, but worth verifying in testing.

3. ActivityItemViewModel refresh logic
In ActivityItemViewModel.swift:77-90, there is logic that searches for RBF replacements by matching isBoosted = true, same txType, same value, and later timestamp. During the window where the original has isBoosted = true but the replacement has not arrived yet, this logic might incorrectly match the original to itself. The 3-attempt retry logic (line 95-99) suggests this was anticipated, which is good.

Performance Considerations

Positive:

• No additional database queries
• No blocking operations added
• The removed log statement (AppViewModel.swift) reduces noise

Suggestion: The removed log was actually useful for debugging. Consider downgrading to .debug level instead of removing it entirely.

Security and Bitcoin Considerations

Secure:

• No changes to actual RBF transaction creation
• No changes to fee calculation
• Proper state transitions maintain data integrity
• Users get better feedback (transaction stays visible during boost)

Test Coverage

Existing tests:

• TxBumpingTests.swift has comprehensive RBF testing (testBumpFeeByRbf)
• Tests verify the RBF transaction is created and confirmed
• Gap: Tests do not verify the isBoosted flag behavior or UI state transitions

Recommendation: Add a unit test to verify the isBoosted state transitions during RBF

UI/UX Impact

User experience improvement:

• Users now see Confirming in ~X min (boosted) instead of transaction disappearing
• Reduces confusion during the event propagation window
• Status icon changes to hourglass with yellow accent

Edge case to verify: If the replacement event never arrives (network issue, node restart, etc.), the original stays in boosted state indefinitely. Is there a timeout or recovery mechanism?

Recommendations

1. Add clarifying comment (optional but helpful) at CoreService.swift:1003
2. Restore debug logging (optional) in AppViewModel.swift
3. Add unit test for isBoosted state transitions during RBF (recommended)
4. Manual testing of the edge cases mentioned above

Verdict: LGTM with minor suggestions

This is a well-targeted fix that improves UX during RBF operations. The code is clean, follows existing patterns, and addresses a real timing issue. The suggestions above are mostly nice-to-haves for clarity and testing robustness.

Approve with optional follow-ups. The PR is ready to merge as-is, with the recommendations being good candidates for future improvements.

[ben-kaufman]

Fix RBF until onchain events arrive

Screen.Recording.2025-11-27.at.11.18.08.AM.mov

[claude]

Pull Request Review: Fix RBF until events arrive

Summary

This PR addresses a timing issue in RBF (Replace-By-Fee) transaction handling. The change ensures that the original transaction remains visible in the UI with a boosting status until the replacement transaction is detected via onchain events, rather than immediately marking it as removed.

Code Quality and Architecture

Positive aspects:

• Clean fix: The solution is minimal and focused, changing only what's necessary
• Symmetric handling: CPFP already used isBoosted = true with boostTxIds.append(), and now RBF uses the same pattern temporarily
• Proper cleanup: When the replacement event arrives, both doesExist = false and isBoosted = false are set (CoreService.swift:359)
• Good logging: Clear log messages explain the state transitions

Changes align with AGENTS.md conventions:

• Uses proper service layer patterns
• Maintains existing Bitcoin/Lightning terminology
• Follows the established activity tracking patterns

Logic Analysis

Before this PR:
In boostOnchainTransaction (RBF path), the code set onchainActivity.doesExist = false immediately

After this PR:
In boostOnchainTransaction (RBF path), the code sets onchainActivity.isBoosted = true (NOT adding to boostTxIds here - that happens in handleOnchainTransactionReplaced)

Why this works:

1. User initiates RBF and original tx is marked with isBoosted = true
2. UI shows Confirming in ~X min (boosted) status (ActivityItemView.swift:277-284)
3. LDK fires onchainTransactionReplaced event
4. Handler marks original as doesExist = false AND isBoosted = false (CoreService.swift:358-359)
5. Replacement tx gets isBoosted = true and boostTxIds updated (CoreService.swift:404-405)

Potential Issues and Questions

1. Inconsistency with CPFP pattern
In CPFP (line 986-987), the code sets both isBoosted = true and appends to boostTxIds. In RBF (line 1003), it only sets isBoosted = true without adding to boostTxIds.

This appears intentional since CPFP parent tx still exists (so it makes sense to add the child txid immediately) while RBF original tx will be replaced (and the replacement gets tracked via the event handler). However, a clarifying comment would be helpful.

2. Race condition consideration
What happens if the onchainTransactionReplaced event arrives before the boostOnchainTransaction update completes? Both operations use ServiceQueue.background(.core), so they should be serialized. This is likely safe, but worth verifying in testing.

3. ActivityItemViewModel refresh logic
In ActivityItemViewModel.swift:77-90, there is logic that searches for RBF replacements by matching isBoosted = true, same txType, same value, and later timestamp. During the window where the original has isBoosted = true but the replacement has not arrived yet, this logic might incorrectly match the original to itself. The 3-attempt retry logic (line 95-99) suggests this was anticipated, which is good.

Performance Considerations

Positive:

• No additional database queries
• No blocking operations added
• The removed log statement (AppViewModel.swift) reduces noise

Suggestion: The removed log was actually useful for debugging. Consider downgrading to .debug level instead of removing it entirely.

Security and Bitcoin Considerations

Secure:

• No changes to actual RBF transaction creation
• No changes to fee calculation
• Proper state transitions maintain data integrity
• Users get better feedback (transaction stays visible during boost)

Test Coverage

Existing tests:

• TxBumpingTests.swift has comprehensive RBF testing (testBumpFeeByRbf)
• Tests verify the RBF transaction is created and confirmed
• Gap: Tests do not verify the isBoosted flag behavior or UI state transitions

Recommendation: Add a unit test to verify the isBoosted state transitions during RBF

UI/UX Impact

User experience improvement:

• Users now see Confirming in ~X min (boosted) instead of transaction disappearing
• Reduces confusion during the event propagation window
• Status icon changes to hourglass with yellow accent

Edge case to verify: If the replacement event never arrives (network issue, node restart, etc.), the original stays in boosted state indefinitely. Is there a timeout or recovery mechanism?

Recommendations

1. Add clarifying comment (optional but helpful) at CoreService.swift:1003
2. Restore debug logging (optional) in AppViewModel.swift
3. Add unit test for isBoosted state transitions during RBF (recommended)
4. Manual testing of the edge cases mentioned above

Verdict: LGTM with minor suggestions

This is a well-targeted fix that improves UX during RBF operations. The code is clean, follows existing patterns, and addresses a real timing issue. The suggestions above are mostly nice-to-haves for clarity and testing robustness.

Approve with optional follow-ups. The PR is ready to merge as-is, with the recommendations being good candidates for future improvements.

[ben-kaufman]

Fix RBF until onchain events arrive

Screen.Recording.2025-11-27.at.11.18.08.AM.mov

[claude]

Pull Request Review: Fix RBF until events arrive

Summary

This PR addresses a timing issue in RBF (Replace-By-Fee) transaction handling. The change ensures that the original transaction remains visible in the UI with a boosting status until the replacement transaction is detected via onchain events, rather than immediately marking it as removed.

Code Quality and Architecture

Positive aspects:

• Clean fix: The solution is minimal and focused, changing only what's necessary
• Symmetric handling: CPFP already used isBoosted = true with boostTxIds.append(), and now RBF uses the same pattern temporarily
• Proper cleanup: When the replacement event arrives, both doesExist = false and isBoosted = false are set (CoreService.swift:359)
• Good logging: Clear log messages explain the state transitions

Changes align with AGENTS.md conventions:

• Uses proper service layer patterns
• Maintains existing Bitcoin/Lightning terminology
• Follows the established activity tracking patterns

Logic Analysis

Before this PR:
In boostOnchainTransaction (RBF path), the code set onchainActivity.doesExist = false immediately

After this PR:
In boostOnchainTransaction (RBF path), the code sets onchainActivity.isBoosted = true (NOT adding to boostTxIds here - that happens in handleOnchainTransactionReplaced)

Why this works:

1. User initiates RBF and original tx is marked with isBoosted = true
2. UI shows Confirming in ~X min (boosted) status (ActivityItemView.swift:277-284)
3. LDK fires onchainTransactionReplaced event
4. Handler marks original as doesExist = false AND isBoosted = false (CoreService.swift:358-359)
5. Replacement tx gets isBoosted = true and boostTxIds updated (CoreService.swift:404-405)

Potential Issues and Questions

1. Inconsistency with CPFP pattern
In CPFP (line 986-987), the code sets both isBoosted = true and appends to boostTxIds. In RBF (line 1003), it only sets isBoosted = true without adding to boostTxIds.

This appears intentional since CPFP parent tx still exists (so it makes sense to add the child txid immediately) while RBF original tx will be replaced (and the replacement gets tracked via the event handler). However, a clarifying comment would be helpful.

2. Race condition consideration
What happens if the onchainTransactionReplaced event arrives before the boostOnchainTransaction update completes? Both operations use ServiceQueue.background(.core), so they should be serialized. This is likely safe, but worth verifying in testing.

3. ActivityItemViewModel refresh logic
In ActivityItemViewModel.swift:77-90, there is logic that searches for RBF replacements by matching isBoosted = true, same txType, same value, and later timestamp. During the window where the original has isBoosted = true but the replacement has not arrived yet, this logic might incorrectly match the original to itself. The 3-attempt retry logic (line 95-99) suggests this was anticipated, which is good.

Performance Considerations

Positive:

• No additional database queries
• No blocking operations added
• The removed log statement (AppViewModel.swift) reduces noise

Suggestion: The removed log was actually useful for debugging. Consider downgrading to .debug level instead of removing it entirely.

Security and Bitcoin Considerations

Secure:

• No changes to actual RBF transaction creation
• No changes to fee calculation
• Proper state transitions maintain data integrity
• Users get better feedback (transaction stays visible during boost)

Test Coverage

Existing tests:

• TxBumpingTests.swift has comprehensive RBF testing (testBumpFeeByRbf)
• Tests verify the RBF transaction is created and confirmed
• Gap: Tests do not verify the isBoosted flag behavior or UI state transitions

Recommendation: Add a unit test to verify the isBoosted state transitions during RBF

UI/UX Impact

User experience improvement:

• Users now see Confirming in ~X min (boosted) instead of transaction disappearing
• Reduces confusion during the event propagation window
• Status icon changes to hourglass with yellow accent

Edge case to verify: If the replacement event never arrives (network issue, node restart, etc.), the original stays in boosted state indefinitely. Is there a timeout or recovery mechanism?

Recommendations

1. Add clarifying comment (optional but helpful) at CoreService.swift:1003
2. Restore debug logging (optional) in AppViewModel.swift
3. Add unit test for isBoosted state transitions during RBF (recommended)
4. Manual testing of the edge cases mentioned above

Verdict: LGTM with minor suggestions

This is a well-targeted fix that improves UX during RBF operations. The code is clean, follows existing patterns, and addresses a real timing issue. The suggestions above are mostly nice-to-haves for clarity and testing robustness.

Approve with optional follow-ups. The PR is ready to merge as-is, with the recommendations being good candidates for future improvements.

[ben-kaufman]

Fix RBF until onchain events arrive

Screen.Recording.2025-11-27.at.11.18.08.AM.mov

[claude]

Pull Request Review: Fix RBF until events arrive

Summary

This PR addresses a timing issue in RBF (Replace-By-Fee) transaction handling. The change ensures that the original transaction remains visible in the UI with a boosting status until the replacement transaction is detected via onchain events, rather than immediately marking it as removed.

Code Quality and Architecture

Positive aspects:

• Clean fix: The solution is minimal and focused, changing only what's necessary
• Symmetric handling: CPFP already used isBoosted = true with boostTxIds.append(), and now RBF uses the same pattern temporarily
• Proper cleanup: When the replacement event arrives, both doesExist = false and isBoosted = false are set (CoreService.swift:359)
• Good logging: Clear log messages explain the state transitions

Changes align with AGENTS.md conventions:

• Uses proper service layer patterns
• Maintains existing Bitcoin/Lightning terminology
• Follows the established activity tracking patterns

Logic Analysis

Before this PR:
In boostOnchainTransaction (RBF path), the code set onchainActivity.doesExist = false immediately

After this PR:
In boostOnchainTransaction (RBF path), the code sets onchainActivity.isBoosted = true (NOT adding to boostTxIds here - that happens in handleOnchainTransactionReplaced)

Why this works:

1. User initiates RBF and original tx is marked with isBoosted = true
2. UI shows Confirming in ~X min (boosted) status (ActivityItemView.swift:277-284)
3. LDK fires onchainTransactionReplaced event
4. Handler marks original as doesExist = false AND isBoosted = false (CoreService.swift:358-359)
5. Replacement tx gets isBoosted = true and boostTxIds updated (CoreService.swift:404-405)

Potential Issues and Questions

1. Inconsistency with CPFP pattern
In CPFP (line 986-987), the code sets both isBoosted = true and appends to boostTxIds. In RBF (line 1003), it only sets isBoosted = true without adding to boostTxIds.

This appears intentional since CPFP parent tx still exists (so it makes sense to add the child txid immediately) while RBF original tx will be replaced (and the replacement gets tracked via the event handler). However, a clarifying comment would be helpful.

2. Race condition consideration
What happens if the onchainTransactionReplaced event arrives before the boostOnchainTransaction update completes? Both operations use ServiceQueue.background(.core), so they should be serialized. This is likely safe, but worth verifying in testing.

3. ActivityItemViewModel refresh logic
In ActivityItemViewModel.swift:77-90, there is logic that searches for RBF replacements by matching isBoosted = true, same txType, same value, and later timestamp. During the window where the original has isBoosted = true but the replacement has not arrived yet, this logic might incorrectly match the original to itself. The 3-attempt retry logic (line 95-99) suggests this was anticipated, which is good.

Performance Considerations

Positive:

• No additional database queries
• No blocking operations added
• The removed log statement (AppViewModel.swift) reduces noise

Suggestion: The removed log was actually useful for debugging. Consider downgrading to .debug level instead of removing it entirely.

Security and Bitcoin Considerations

Secure:

• No changes to actual RBF transaction creation
• No changes to fee calculation
• Proper state transitions maintain data integrity
• Users get better feedback (transaction stays visible during boost)

Test Coverage

Existing tests:

• TxBumpingTests.swift has comprehensive RBF testing (testBumpFeeByRbf)
• Tests verify the RBF transaction is created and confirmed
• Gap: Tests do not verify the isBoosted flag behavior or UI state transitions

Recommendation: Add a unit test to verify the isBoosted state transitions during RBF

UI/UX Impact

User experience improvement:

• Users now see Confirming in ~X min (boosted) instead of transaction disappearing
• Reduces confusion during the event propagation window
• Status icon changes to hourglass with yellow accent

Edge case to verify: If the replacement event never arrives (network issue, node restart, etc.), the original stays in boosted state indefinitely. Is there a timeout or recovery mechanism?

Recommendations

1. Add clarifying comment (optional but helpful) at CoreService.swift:1003
2. Restore debug logging (optional) in AppViewModel.swift
3. Add unit test for isBoosted state transitions during RBF (recommended)
4. Manual testing of the edge cases mentioned above

Verdict: LGTM with minor suggestions

This is a well-targeted fix that improves UX during RBF operations. The code is clean, follows existing patterns, and addresses a real timing issue. The suggestions above are mostly nice-to-haves for clarity and testing robustness.

Approve with optional follow-ups. The PR is ready to merge as-is, with the recommendations being good candidates for future improvements.

[ben-kaufman]

Fix RBF until onchain events arrive

Screen.Recording.2025-11-27.at.11.18.08.AM.mov

[claude]

Pull Request Review: Fix RBF until events arrive

Summary

This PR addresses a timing issue in RBF (Replace-By-Fee) transaction handling. The change ensures that the original transaction remains visible in the UI with a boosting status until the replacement transaction is detected via onchain events, rather than immediately marking it as removed.

Code Quality and Architecture

Positive aspects:

• Clean fix: The solution is minimal and focused, changing only what's necessary
• Symmetric handling: CPFP already used isBoosted = true with boostTxIds.append(), and now RBF uses the same pattern temporarily
• Proper cleanup: When the replacement event arrives, both doesExist = false and isBoosted = false are set (CoreService.swift:359)
• Good logging: Clear log messages explain the state transitions

Changes align with AGENTS.md conventions:

• Uses proper service layer patterns
• Maintains existing Bitcoin/Lightning terminology
• Follows the established activity tracking patterns

Logic Analysis

Before this PR:
In boostOnchainTransaction (RBF path), the code set onchainActivity.doesExist = false immediately

After this PR:
In boostOnchainTransaction (RBF path), the code sets onchainActivity.isBoosted = true (NOT adding to boostTxIds here - that happens in handleOnchainTransactionReplaced)

Why this works:

1. User initiates RBF and original tx is marked with isBoosted = true
2. UI shows Confirming in ~X min (boosted) status (ActivityItemView.swift:277-284)
3. LDK fires onchainTransactionReplaced event
4. Handler marks original as doesExist = false AND isBoosted = false (CoreService.swift:358-359)
5. Replacement tx gets isBoosted = true and boostTxIds updated (CoreService.swift:404-405)

Potential Issues and Questions

1. Inconsistency with CPFP pattern
In CPFP (line 986-987), the code sets both isBoosted = true and appends to boostTxIds. In RBF (line 1003), it only sets isBoosted = true without adding to boostTxIds.

This appears intentional since CPFP parent tx still exists (so it makes sense to add the child txid immediately) while RBF original tx will be replaced (and the replacement gets tracked via the event handler). However, a clarifying comment would be helpful.

2. Race condition consideration
What happens if the onchainTransactionReplaced event arrives before the boostOnchainTransaction update completes? Both operations use ServiceQueue.background(.core), so they should be serialized. This is likely safe, but worth verifying in testing.

3. ActivityItemViewModel refresh logic
In ActivityItemViewModel.swift:77-90, there is logic that searches for RBF replacements by matching isBoosted = true, same txType, same value, and later timestamp. During the window where the original has isBoosted = true but the replacement has not arrived yet, this logic might incorrectly match the original to itself. The 3-attempt retry logic (line 95-99) suggests this was anticipated, which is good.

Performance Considerations

Positive:

• No additional database queries
• No blocking operations added
• The removed log statement (AppViewModel.swift) reduces noise

Suggestion: The removed log was actually useful for debugging. Consider downgrading to .debug level instead of removing it entirely.

Security and Bitcoin Considerations

Secure:

• No changes to actual RBF transaction creation
• No changes to fee calculation
• Proper state transitions maintain data integrity
• Users get better feedback (transaction stays visible during boost)

Test Coverage

Existing tests:

• TxBumpingTests.swift has comprehensive RBF testing (testBumpFeeByRbf)
• Tests verify the RBF transaction is created and confirmed
• Gap: Tests do not verify the isBoosted flag behavior or UI state transitions

Recommendation: Add a unit test to verify the isBoosted state transitions during RBF

UI/UX Impact

User experience improvement:

• Users now see Confirming in ~X min (boosted) instead of transaction disappearing
• Reduces confusion during the event propagation window
• Status icon changes to hourglass with yellow accent

Edge case to verify: If the replacement event never arrives (network issue, node restart, etc.), the original stays in boosted state indefinitely. Is there a timeout or recovery mechanism?

Recommendations

1. Add clarifying comment (optional but helpful) at CoreService.swift:1003
2. Restore debug logging (optional) in AppViewModel.swift
3. Add unit test for isBoosted state transitions during RBF (recommended)
4. Manual testing of the edge cases mentioned above

Verdict: LGTM with minor suggestions

This is a well-targeted fix that improves UX during RBF operations. The code is clean, follows existing patterns, and addresses a real timing issue. The suggestions above are mostly nice-to-haves for clarity and testing robustness.

Approve with optional follow-ups. The PR is ready to merge as-is, with the recommendations being good candidates for future improvements.

[ben-kaufman]

Fix RBF until onchain events arrive

Screen.Recording.2025-11-27.at.11.18.08.AM.mov

[claude]

Pull Request Review: Fix RBF until events arrive

Summary

This PR addresses a timing issue in RBF (Replace-By-Fee) transaction handling. The change ensures that the original transaction remains visible in the UI with a boosting status until the replacement transaction is detected via onchain events, rather than immediately marking it as removed.

Code Quality and Architecture

Positive aspects:

• Clean fix: The solution is minimal and focused, changing only what's necessary
• Symmetric handling: CPFP already used isBoosted = true with boostTxIds.append(), and now RBF uses the same pattern temporarily
• Proper cleanup: When the replacement event arrives, both doesExist = false and isBoosted = false are set (CoreService.swift:359)
• Good logging: Clear log messages explain the state transitions

Changes align with AGENTS.md conventions:

• Uses proper service layer patterns
• Maintains existing Bitcoin/Lightning terminology
• Follows the established activity tracking patterns

Logic Analysis

Before this PR:
In boostOnchainTransaction (RBF path), the code set onchainActivity.doesExist = false immediately

After this PR:
In boostOnchainTransaction (RBF path), the code sets onchainActivity.isBoosted = true (NOT adding to boostTxIds here - that happens in handleOnchainTransactionReplaced)

Why this works:

1. User initiates RBF and original tx is marked with isBoosted = true
2. UI shows Confirming in ~X min (boosted) status (ActivityItemView.swift:277-284)
3. LDK fires onchainTransactionReplaced event
4. Handler marks original as doesExist = false AND isBoosted = false (CoreService.swift:358-359)
5. Replacement tx gets isBoosted = true and boostTxIds updated (CoreService.swift:404-405)

Potential Issues and Questions

1. Inconsistency with CPFP pattern
In CPFP (line 986-987), the code sets both isBoosted = true and appends to boostTxIds. In RBF (line 1003), it only sets isBoosted = true without adding to boostTxIds.

This appears intentional since CPFP parent tx still exists (so it makes sense to add the child txid immediately) while RBF original tx will be replaced (and the replacement gets tracked via the event handler). However, a clarifying comment would be helpful.

2. Race condition consideration
What happens if the onchainTransactionReplaced event arrives before the boostOnchainTransaction update completes? Both operations use ServiceQueue.background(.core), so they should be serialized. This is likely safe, but worth verifying in testing.

3. ActivityItemViewModel refresh logic
In ActivityItemViewModel.swift:77-90, there is logic that searches for RBF replacements by matching isBoosted = true, same txType, same value, and later timestamp. During the window where the original has isBoosted = true but the replacement has not arrived yet, this logic might incorrectly match the original to itself. The 3-attempt retry logic (line 95-99) suggests this was anticipated, which is good.

Performance Considerations

Positive:

• No additional database queries
• No blocking operations added
• The removed log statement (AppViewModel.swift) reduces noise

Suggestion: The removed log was actually useful for debugging. Consider downgrading to .debug level instead of removing it entirely.

Security and Bitcoin Considerations

Secure:

• No changes to actual RBF transaction creation
• No changes to fee calculation
• Proper state transitions maintain data integrity
• Users get better feedback (transaction stays visible during boost)

Test Coverage

Existing tests:

• TxBumpingTests.swift has comprehensive RBF testing (testBumpFeeByRbf)
• Tests verify the RBF transaction is created and confirmed
• Gap: Tests do not verify the isBoosted flag behavior or UI state transitions

Recommendation: Add a unit test to verify the isBoosted state transitions during RBF

UI/UX Impact

User experience improvement:

• Users now see Confirming in ~X min (boosted) instead of transaction disappearing
• Reduces confusion during the event propagation window
• Status icon changes to hourglass with yellow accent

Edge case to verify: If the replacement event never arrives (network issue, node restart, etc.), the original stays in boosted state indefinitely. Is there a timeout or recovery mechanism?

Recommendations

1. Add clarifying comment (optional but helpful) at CoreService.swift:1003
2. Restore debug logging (optional) in AppViewModel.swift
3. Add unit test for isBoosted state transitions during RBF (recommended)
4. Manual testing of the edge cases mentioned above

Verdict: LGTM with minor suggestions

This is a well-targeted fix that improves UX during RBF operations. The code is clean, follows existing patterns, and addresses a real timing issue. The suggestions above are mostly nice-to-haves for clarity and testing robustness.

Approve with optional follow-ups. The PR is ready to merge as-is, with the recommendations being good candidates for future improvements.