Fix restart after alternative feerate remote commit #847
Conversation
When our peer broadcasts a commit tx that uses an alternative feerate, we failed to republish our main transaction because of a useless `require()` in the corresponding function. We remove this `require`, since it shouldn't prevent us from claiming our main output. Note that this can only happen for anchor outputs channels, since we stopped sending signatures for alternative feerates in the latest release.
| else -> logger.error(t) { "error on command ${cmd::class.simpleName}" } | ||
| is ChannelCommand.MessageReceived -> logger.error(t) { "error on message ${cmd.message::class.simpleName}: ${t.message}" } | ||
| is ChannelCommand.WatchReceived -> logger.error { "error on watch event ${cmd.watch::class.simpleName}: ${t.message}" } | ||
| else -> logger.error(t) { "error on command ${cmd::class.simpleName}: ${t.message}" } |
There was a problem hiding this comment.
I'm re-adding the exception message directly here because it looks like our logger is swallowing the exception, and thus missing useful data during debugging.
There was a problem hiding this comment.
You mean in our tests? Or in phoenix app logs?
There was a problem hiding this comment.
In the Phoenix logs: I'm seeing error on command Restore without details about the exception. The exception is correctly printed in test logs though.
There was a problem hiding this comment.
The bug is in phoenix iOS, the throwable is dropped here: https://github.com/ACINQ/phoenix/blob/5061fd25f45360aa71d232e5b7ae4e6c424eb94d/phoenix-shared/src/iosMain/kotlin/fr/acinq/phoenix/utils/PassthruLogWriter.kt#L15
There was a problem hiding this comment.
I remember old discussions about logs containing exceptions being too large, which created issues...isn't that why it was explicitly dropped? Maybe @dpad85 or @robbiehanson remembers?
There was a problem hiding this comment.
Acking the PR in the meantime as this is a separate issue.
2 participants
When our peer broadcasts a commit tx that uses an alternative feerate, we failed to republish our main transaction because of a useless
require()in the corresponding function. We remove thisrequire, since it shouldn't prevent us from claiming our main output.Note that this can only happen for anchor outputs channels, since we stopped sending signatures for alternative feerates in the latest release.