Address pending signature retry review feedback
What the developer wrote
Authored by rockstardev
Address pending signature retry review feedback
What changed, and why it matters
This commit is a follow-up code review patch for BTCPay Server's pending Bitcoin transaction (multisig) signing feature. It tightens when the service logs a warning versus a debug message after a failed finalization attempt, and it removes an unused optional parameter from an internal helper method. The changes are defensive: they reduce false-warning noise and make the per-input signature tracking more accurate. There is no direct evidence in the commit of an exploitable vulnerability being fixed.
Treat as a normal code-quality/review-feedback patch. Review the broader pending transaction signing flow to ensure SignaturesNeeded is always set correctly and that finalization errors are escalated appropriately. No urgent security deployment is indicated by this commit alone.
Security signals we found
Logging level change reduces false-warning noise but could mask real finalization failures if SignaturesNeeded is incorrectly zero
Removal of additionalPsbt parameter prevents accidental combination of an unvalidated extra PSBT into the effective PSBT
Per-input signature progress tracking refined to avoid prematurely counting aggregate signatures
New test asserts non-multisig finalization failures are logged at Debug, not Warning
No explicit vulnerability disclosure or CVE referenced in commit or supplied references
Evidence from the diff
The commit addresses review feedback on PendingTransactionService finalization/retry logic. Key changes: (1) log level selection for finalization errors now only uses LogLevel.Warning when SignaturesNeeded > 0 and SignaturesCollected >= SignaturesNeeded; previously it warned whenever collected >= needed, which for single-sig or zero-needed cases could emit misleading warnings. (2) BuildEffectivePsbt drops the unused additionalPsbt parameter, so the effective PSBT is built only from stored collected signatures, not from a freshly passed PSBT. (3) Tests are updated/added to assert that non-multisig finalization failures log at Debug, that per-input progress is retained, and that a 2-of-4 multisig only reaches Signed state after enough distinct inputs are signed. The patch is partial/refinement rather than a complete security fix.
Changed components
BTCPayServer/HostedServices/PendingTransactionService.csBTCPayServer.Tests/MultisigTests.csInspect captured patch +109 / −10
diff --git a/BTCPayServer.Tests/MultisigTests.cs b/BTCPayServer.Tests/MultisigTests.cs
index 105c38e..c6dc1bc 100644
--- a/BTCPayServer.Tests/MultisigTests.cs
+++ b/BTCPayServer.Tests/MultisigTests.cs
@@ -1,4 +1,5 @@
using System;
+using System.Collections.Generic;
using System.Linq;
using System.Text.RegularExpressions;
using System.Threading;
@@ -258,6 +259,46 @@ public class MultisigTests(ITestOutputHelper helper) : UnitTestBase(helper)
Assert.Equal(1, blob.SignaturesCollected);
}
+ [Fact]
+ [Trait("Integration", "Integration")]
+ public async Task PendingNonMultisigFinalizationFailureLogsAtDebugLevel()
+ {
+ var dbTester = CreateDBTester();
+ await dbTester.MigrateAsync();
+ var contextFactory = dbTester.CreateContextFactory();
+ var storeId = await CreateTestStore(contextFactory);
+ var logger = new RecordingLogger<PendingTransactionService>();
+ var pendingTransactionService = new PendingTransactionService(
+ CreateNetworkProvider(),
+ contextFactory,
+ new EventAggregator(BTCPayLogs),
+ logger);
+ var testPsbt = CreatePendingSingleSigPsbt();
+ var pendingTransaction = await pendingTransactionService.CreatePendingTransaction(
+ storeId,
+ "BTC",
+ testPsbt.BasePsbt,
+ RequestBaseUrl.FromUrl("https://example.com"),
+ cancellationToken: CancellationToken.None);
+
+ pendingTransaction = await pendingTransactionService.CollectSignature(
+ new PendingTransactionService.PendingTransactionFullId("BTC", storeId, pendingTransaction.Id),
+ SignInputs(testPsbt.BasePsbt, testPsbt.Signer, 0),
+ CancellationToken.None);
+
+ Assert.NotNull(pendingTransaction);
+ var blob = pendingTransaction.GetBlob();
+ Assert.Equal(0, blob.SignaturesNeeded);
+ Assert.Equal(0, blob.SignaturesCollected);
+ Assert.Equal(PendingTransactionState.Pending, pendingTransaction.State);
+ Assert.Contains(logger.Entries, entry =>
+ entry.Level == LogLevel.Debug &&
+ entry.Message.Contains(pendingTransaction.Id, StringComparison.Ordinal));
+ Assert.DoesNotContain(logger.Entries, entry =>
+ entry.Level == LogLevel.Warning &&
+ entry.Message.Contains(pendingTransaction.Id, StringComparison.Ordinal));
+ }
+
[Fact]
[Trait("Integration", "Integration")]
public async Task PendingTwoOfFourMultisigRetainsProgressingPSBTsAndRetriesFinalization()
@@ -266,11 +307,12 @@ public class MultisigTests(ITestOutputHelper helper) : UnitTestBase(helper)
await dbTester.MigrateAsync();
var contextFactory = dbTester.CreateContextFactory();
var storeId = await CreateTestStore(contextFactory);
+ var logger = new RecordingLogger<PendingTransactionService>();
var pendingTransactionService = new PendingTransactionService(
CreateNetworkProvider(),
contextFactory,
new EventAggregator(BTCPayLogs),
- LoggerFactory.CreateLogger<PendingTransactionService>());
+ logger);
var testPsbt = CreatePendingMultisigPsbt(4);
var pendingTransaction = await pendingTransactionService.CreatePendingTransaction(
storeId,
@@ -319,15 +361,27 @@ public class MultisigTests(ITestOutputHelper helper) : UnitTestBase(helper)
Assert.Equal(2, blob.CollectedSignatures.Count);
Assert.Equal(2, blob.SignaturesCollected);
Assert.Equal(PendingTransactionState.Pending, pendingTransaction.State);
+ Assert.Contains(logger.Entries, entry =>
+ entry.Level == LogLevel.Warning &&
+ entry.Message.Contains(pendingTransaction.Id, StringComparison.Ordinal));
- // A third eligible signer must be retained, reported as 3/4, and trigger
- // another finalization attempt. NBitcoin can now select two valid signatures.
+ // Per-input progress must be retained even while the aggregate remains at two.
pendingTransaction = await pendingTransactionService.CollectSignature(
pendingTransactionId,
- SignInputs(testPsbt.BasePsbt, testPsbt.SignerC, 0, 1),
+ SignInputs(testPsbt.BasePsbt, testPsbt.Signers[2], 0),
CancellationToken.None);
blob = pendingTransaction!.GetBlob();
Assert.Equal(3, blob.CollectedSignatures.Count);
+ Assert.Equal(2, blob.SignaturesCollected);
+ Assert.Equal(PendingTransactionState.Pending, pendingTransaction.State);
+
+ // Once the third signer reaches the second input, NBitcoin can select two valid signatures.
+ pendingTransaction = await pendingTransactionService.CollectSignature(
+ pendingTransactionId,
+ SignInputs(testPsbt.BasePsbt, testPsbt.Signers[2], 1),
+ CancellationToken.None);
+ blob = pendingTransaction!.GetBlob();
+ Assert.Equal(4, blob.CollectedSignatures.Count);
Assert.Equal(3, blob.SignaturesCollected);
Assert.Equal(PendingTransactionState.Signed, pendingTransaction.State);
}
@@ -824,6 +878,28 @@ public class MultisigTests(ITestOutputHelper helper) : UnitTestBase(helper)
return psbt.ToBase64();
}
+ private static (PSBT BasePsbt, Key Signer) CreatePendingSingleSigPsbt()
+ {
+ var network = Network.RegTest;
+ var signer = new Key();
+ var scriptPubKey = signer.PubKey.WitHash.ScriptPubKey;
+
+ var previousTransactionA = network.CreateTransaction();
+ previousTransactionA.Outputs.Add(Money.Coins(1.0m), scriptPubKey);
+ var previousTransactionB = network.CreateTransaction();
+ previousTransactionB.Outputs.Add(Money.Coins(1.1m), scriptPubKey);
+
+ var builder = network.CreateTransactionBuilder();
+ builder.AddCoins(
+ previousTransactionA.Outputs.AsCoins().First(),
+ previousTransactionB.Outputs.AsCoins().First());
+ builder.Send(new Key().PubKey.WitHash.ScriptPubKey, Money.Coins(1.5m));
+ builder.SetChange(new Key().PubKey.WitHash.ScriptPubKey);
+ builder.SendFees(Money.Satoshis(10_000));
+
+ return (builder.BuildPSBT(false), signer);
+ }
+
private static TestPendingMultisigPsbt CreatePendingMultisigPsbt(int signerCount = 3)
{
if (signerCount < 2)
@@ -898,10 +974,34 @@ public class MultisigTests(ITestOutputHelper helper) : UnitTestBase(helper)
return new StoreRepository(contextFactory, new JsonSerializerSettings(), eventAggregator, settingsRepository);
}
+ private sealed class RecordingLogger<T> : ILogger<T>
+ {
+ public List<(LogLevel Level, string Message)> Entries { get; } = [];
+
+ public IDisposable BeginScope<TState>(TState state)
+ {
+ return null;
+ }
+
+ public bool IsEnabled(LogLevel logLevel)
+ {
+ return true;
+ }
+
+ public void Log<TState>(
+ LogLevel logLevel,
+ EventId eventId,
+ TState state,
+ Exception exception,
+ Func<TState, Exception, string> formatter)
+ {
+ Entries.Add((logLevel, formatter(state, exception)));
+ }
+ }
+
private sealed record TestPendingMultisigPsbt(PSBT BasePsbt, Key[] Signers)
{
public Key SignerA => Signers[0];
public Key SignerB => Signers[1];
- public Key SignerC => Signers[2];
}
}
diff --git a/BTCPayServer/HostedServices/PendingTransactionService.cs b/BTCPayServer/HostedServices/PendingTransactionService.cs
index 486286e..fd96a40 100644
--- a/BTCPayServer/HostedServices/PendingTransactionService.cs
+++ b/BTCPayServer/HostedServices/PendingTransactionService.cs
@@ -237,7 +237,9 @@ public class PendingTransactionService(
else
{
var finalizationErrorDetails = FormatFinalizationErrors(finalizationErrors);
- var logLevel = (blob.SignaturesCollected ?? 0) >= (blob.SignaturesNeeded ?? int.MaxValue)
+ var signaturesNeeded = blob.SignaturesNeeded ?? 0;
+ var logLevel = signaturesNeeded > 0 &&
+ (blob.SignaturesCollected ?? 0) >= signaturesNeeded
? LogLevel.Warning
: LogLevel.Debug;
logger.Log(
@@ -358,7 +360,7 @@ public class PendingTransactionService(
return true;
}
- private static PSBT BuildEffectivePsbt(PendingTransactionBlob blob, Network network, PSBT? additionalPsbt = null)
+ private static PSBT BuildEffectivePsbt(PendingTransactionBlob blob, Network network)
{
var effectivePsbt = PSBT.Parse(blob.PSBT, network);
foreach (var collectedSignature in blob.CollectedSignatures)
@@ -366,9 +368,6 @@ public class PendingTransactionService(
effectivePsbt.Combine(PSBT.Parse(collectedSignature.ReceivedPSBT, network));
}
- if (additionalPsbt is not null)
- effectivePsbt.Combine(additionalPsbt);
-
return effectivePsbt;
}
Why this scored 26/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.