From a5fe54a1ee942593c6221b04682083fc9abf8824 Mon Sep 17 00:00:00 2001 From: zeptin Date: Mon, 30 Nov 2020 13:03:53 +0000 Subject: [PATCH] Add consolidate endpoint and adjust subtractfee logic (#235) * Add consolidate endpoint and adjust subtractfee logic * Fix and augment tests * Update src/Stratis.Bitcoin.Features.Wallet/Services/WalletService.cs --- src/NBitcoin/TransactionBuilder.cs | 4 +- .../SmartContractWalletTransactionHandler.cs | 3 +- .../WalletTransactionHandlerTest.cs | 135 +++++++++++++++++- .../Controllers/WalletController.cs | 9 ++ .../Interfaces/IWalletTransactionHandler.cs | 2 + .../Models/RequestModels.cs | 39 +++++ .../Services/IWalletService.cs | 3 + .../Services/WalletService.cs | 95 ++++++++++++ .../WalletTransactionHandler.cs | 43 +++++- 9 files changed, 322 insertions(+), 11 deletions(-) diff --git a/src/NBitcoin/TransactionBuilder.cs b/src/NBitcoin/TransactionBuilder.cs index 5d1506d0cd..f7bd466f7a 100644 --- a/src/NBitcoin/TransactionBuilder.cs +++ b/src/NBitcoin/TransactionBuilder.cs @@ -1545,7 +1545,7 @@ public ICoin[] FindSpentCoins(Transaction tx) /// Estimate the physical size of the transaction /// /// The transaction to be estimated - /// + /// The estimated size of the transaction in bytes. public int EstimateSize(Transaction tx) { return EstimateSize(tx, false); @@ -1617,7 +1617,7 @@ private void EstimateScriptSigSize(ICoin coin, ref int witSize, ref int baseSize } if (scriptSigSize == -1) - scriptSigSize += coin.TxOut.ScriptPubKey.Length; //Using heurestic to approximate size of unknown scriptPubKey + scriptSigSize += coin.TxOut.ScriptPubKey.Length; //Using heuristic to approximate size of unknown scriptPubKey if (coin.GetHashVersion(this.Network) == HashVersion.Witness) witSize += scriptSigSize + 1; //Account for the push diff --git a/src/Stratis.Bitcoin.Features.SmartContracts/Wallet/SmartContractWalletTransactionHandler.cs b/src/Stratis.Bitcoin.Features.SmartContracts/Wallet/SmartContractWalletTransactionHandler.cs index fee317ffd6..414ed73b6f 100644 --- a/src/Stratis.Bitcoin.Features.SmartContracts/Wallet/SmartContractWalletTransactionHandler.cs +++ b/src/Stratis.Bitcoin.Features.SmartContracts/Wallet/SmartContractWalletTransactionHandler.cs @@ -57,8 +57,9 @@ protected override void AddRecipients(TransactionBuildContext context) if (context.Recipients.Any(recipient => recipient.Amount == Money.Zero && !recipient.ScriptPubKey.IsSmartContractExec())) throw new WalletException("No amount specified."); + // TODO: Port the necessary logic from the regular wallet transaction handler if (context.Recipients.Any(a => a.SubtractFeeFromAmount)) - throw new NotImplementedException("Substracting the fee from the recipient is not supported yet."); + throw new NotImplementedException("Subtracting the fee from the recipient is not supported yet."); foreach (Recipient recipient in context.Recipients) context.TransactionBuilder.Send(recipient.ScriptPubKey, recipient.Amount); diff --git a/src/Stratis.Bitcoin.Features.Wallet.Tests/WalletTransactionHandlerTest.cs b/src/Stratis.Bitcoin.Features.Wallet.Tests/WalletTransactionHandlerTest.cs index 5d3e2dfd2c..112ba5a6ab 100644 --- a/src/Stratis.Bitcoin.Features.Wallet.Tests/WalletTransactionHandlerTest.cs +++ b/src/Stratis.Bitcoin.Features.Wallet.Tests/WalletTransactionHandlerTest.cs @@ -620,7 +620,7 @@ public void When_BuildTransactionIsCalled_Then_FeeIsDeductedFromAmountsInTransac { AccountReference = walletReference, MinConfirmations = 0, - FeeType = FeeType.Low, + TransactionFee = Money.Coins(0.0001m), WalletPassword = "password", Recipients = new[] { @@ -636,6 +636,139 @@ public void When_BuildTransactionIsCalled_Then_FeeIsDeductedFromAmountsInTransac Assert.True(transaction.Outputs.Count(i => i.Value.Satoshi < 5_000_000_000) == 2); // 2 outputs should have fees taken from the amount } + [Fact] + public void When_BuildTransactionIsCalledWithoutTransactionFee_Then_FeeIsDeductedFromSingleOutputInTransaction() + { + DataFolder dataFolder = CreateDataFolder(this); + + IWalletRepository walletRepository = new SQLiteWalletRepository(this.LoggerFactory.Object, dataFolder, this.Network, DateTimeProvider.Default, new ScriptAddressReader()) + { + TestMode = true + }; + + var walletFeePolicy = new Mock(); + walletFeePolicy.Setup(w => w.GetFeeRate(FeeType.Low.ToConfirmations())).Returns(new FeeRate(20000)); + + var walletManager = new WalletManager(this.LoggerFactory.Object, this.Network, new ChainIndexer(this.Network), new WalletSettings(NodeSettings.Default(this.Network)), + dataFolder, walletFeePolicy.Object, new Mock().Object, new NodeLifetime(), DateTimeProvider.Default, this.scriptAddressReader, walletRepository); + + walletManager.Start(); + + var reserveUtxoService = new ReserveUtxoService(this.loggerFactory, new Mock().Object); + + var walletTransactionHandler = new WalletTransactionHandler(this.LoggerFactory.Object, walletManager, walletFeePolicy.Object, this.Network, this.standardTransactionPolicy, reserveUtxoService); + + (Wallet wallet, ExtKey extKey) = WalletTestsHelpers.GenerateBlankWalletWithExtKey("myWallet1", "password", walletRepository); + + walletManager.Wallets.Add(wallet); + + int accountIndex = 0; + ExtKey addressExtKey = extKey.Derive(new KeyPath($"m/44'/{this.Network.Consensus.CoinType}'/{accountIndex}'")); + ExtPubKey extPubKey = addressExtKey.Neuter(); + + HdAccount account = wallet.AddNewAccount(extPubKey, accountName: "account1"); + + var address = account.ExternalAddresses.First(); + var destination = account.InternalAddresses.First(); + var destination2 = account.InternalAddresses.Skip(1).First(); + var destination3 = account.InternalAddresses.Skip(2).First(); + + // Wallet with 4 coinbase outputs of 50 = 200. + var chain = new ChainIndexer(wallet.Network); + WalletTestsHelpers.AddBlocksWithCoinbaseToChain(wallet.Network, chain, address, 4); + + var walletReference = new WalletAccountReference + { + AccountName = "account1", + WalletName = "myWallet1" + }; + + // Create a transaction with 3 outputs 50 + 50 + 50 = 150 but with fees charged to recipients. + var context = new TransactionBuildContext(this.Network) + { + AccountReference = walletReference, + MinConfirmations = 0, + FeeType = FeeType.Low, + WalletPassword = "password", + Recipients = new[] + { + new Recipient { Amount = new Money(50, MoneyUnit.BTC), ScriptPubKey = destination.ScriptPubKey, SubtractFeeFromAmount = true }, + new Recipient { Amount = new Money(50, MoneyUnit.BTC), ScriptPubKey = destination2.ScriptPubKey, SubtractFeeFromAmount = false }, + new Recipient { Amount = new Money(50, MoneyUnit.BTC), ScriptPubKey = destination3.ScriptPubKey, SubtractFeeFromAmount = false } + }.ToList() + }; + + Transaction transaction = walletTransactionHandler.BuildTransaction(context); + Assert.Equal(3, transaction.Inputs.Count); // 3 inputs + Assert.Equal(3, transaction.Outputs.Count); // 3 outputs with change + Assert.True(transaction.Outputs.Count(i => i.Value.Satoshi < 5_000_000_000) == 1); // 1 output should have fees taken from the amount + } + + [Fact] + public void When_BuildTransactionIsCalledWithoutTransactionFee_Then_MultipleSubtractFeeRecipients_ThrowsException() + { + DataFolder dataFolder = CreateDataFolder(this); + + IWalletRepository walletRepository = new SQLiteWalletRepository(this.LoggerFactory.Object, dataFolder, this.Network, DateTimeProvider.Default, new ScriptAddressReader()) + { + TestMode = true + }; + + var walletFeePolicy = new Mock(); + walletFeePolicy.Setup(w => w.GetFeeRate(FeeType.Low.ToConfirmations())).Returns(new FeeRate(20000)); + + var walletManager = new WalletManager(this.LoggerFactory.Object, this.Network, new ChainIndexer(this.Network), new WalletSettings(NodeSettings.Default(this.Network)), + dataFolder, walletFeePolicy.Object, new Mock().Object, new NodeLifetime(), DateTimeProvider.Default, this.scriptAddressReader, walletRepository); + + walletManager.Start(); + + var reserveUtxoService = new ReserveUtxoService(this.loggerFactory, new Mock().Object); + + var walletTransactionHandler = new WalletTransactionHandler(this.LoggerFactory.Object, walletManager, walletFeePolicy.Object, this.Network, this.standardTransactionPolicy, reserveUtxoService); + + (Wallet wallet, ExtKey extKey) = WalletTestsHelpers.GenerateBlankWalletWithExtKey("myWallet1", "password", walletRepository); + + walletManager.Wallets.Add(wallet); + + int accountIndex = 0; + ExtKey addressExtKey = extKey.Derive(new KeyPath($"m/44'/{this.Network.Consensus.CoinType}'/{accountIndex}'")); + ExtPubKey extPubKey = addressExtKey.Neuter(); + + HdAccount account = wallet.AddNewAccount(extPubKey, accountName: "account1"); + + var address = account.ExternalAddresses.First(); + var destination = account.InternalAddresses.First(); + var destination2 = account.InternalAddresses.Skip(1).First(); + var destination3 = account.InternalAddresses.Skip(2).First(); + + // Wallet with 4 coinbase outputs of 50 = 200. + var chain = new ChainIndexer(wallet.Network); + WalletTestsHelpers.AddBlocksWithCoinbaseToChain(wallet.Network, chain, address, 4); + + var walletReference = new WalletAccountReference + { + AccountName = "account1", + WalletName = "myWallet1" + }; + + // Create a transaction with 3 outputs 50 + 50 + 50 = 150 but with fees charged to recipients. + var context = new TransactionBuildContext(this.Network) + { + AccountReference = walletReference, + MinConfirmations = 0, + FeeType = FeeType.Low, + WalletPassword = "password", + Recipients = new[] + { + new Recipient { Amount = new Money(50, MoneyUnit.BTC), ScriptPubKey = destination.ScriptPubKey, SubtractFeeFromAmount = true }, + new Recipient { Amount = new Money(50, MoneyUnit.BTC), ScriptPubKey = destination2.ScriptPubKey, SubtractFeeFromAmount = true }, + new Recipient { Amount = new Money(50, MoneyUnit.BTC), ScriptPubKey = destination3.ScriptPubKey, SubtractFeeFromAmount = false } + }.ToList() + }; + + Assert.Throws(() => walletTransactionHandler.BuildTransaction(context)); + } + public static TransactionBuildContext CreateContext(Network network, WalletAccountReference accountReference, string password, Script destinationScript, Money amount, FeeType feeType, int minConfirmations, string opReturnData = null, List recipients = null) { diff --git a/src/Stratis.Bitcoin.Features.Wallet/Controllers/WalletController.cs b/src/Stratis.Bitcoin.Features.Wallet/Controllers/WalletController.cs index 8692ef5848..3a71fb5455 100644 --- a/src/Stratis.Bitcoin.Features.Wallet/Controllers/WalletController.cs +++ b/src/Stratis.Bitcoin.Features.Wallet/Controllers/WalletController.cs @@ -829,6 +829,15 @@ public async Task OfflineSignRequest([FromBody] OfflineSignReques return await this.Execute(request, cancellationToken, async (req, token) => this.Json(await this.walletService.OfflineSignRequest(req, token))); } + [HttpPost] + [Route("consolidate")] + public async Task Consolidate([FromBody] ConsolidationRequest request, + CancellationToken cancellationToken = default(CancellationToken)) + { + return await this.Execute(request, cancellationToken, + async (req, token) => this.Json(await this.walletService.Consolidate(req, token))); + } + private TransactionItemModel FindSimilarReceivedTransactionOutput(List items, TransactionData transaction) { diff --git a/src/Stratis.Bitcoin.Features.Wallet/Interfaces/IWalletTransactionHandler.cs b/src/Stratis.Bitcoin.Features.Wallet/Interfaces/IWalletTransactionHandler.cs index fd3d45b30a..2abd265982 100644 --- a/src/Stratis.Bitcoin.Features.Wallet/Interfaces/IWalletTransactionHandler.cs +++ b/src/Stratis.Bitcoin.Features.Wallet/Interfaces/IWalletTransactionHandler.cs @@ -40,5 +40,7 @@ public interface IWalletTransactionHandler /// The context that is used to build a new transaction. /// The estimated fee. Money EstimateFee(TransactionBuildContext context); + + int EstimateSize(TransactionBuildContext context); } } diff --git a/src/Stratis.Bitcoin.Features.Wallet/Models/RequestModels.cs b/src/Stratis.Bitcoin.Features.Wallet/Models/RequestModels.cs index cc64891e62..2869060eb5 100644 --- a/src/Stratis.Bitcoin.Features.Wallet/Models/RequestModels.cs +++ b/src/Stratis.Bitcoin.Features.Wallet/Models/RequestModels.cs @@ -989,4 +989,43 @@ public class SweepRequest : RequestModel public bool Broadcast { get; set; } } + + public sealed class ConsolidationRequest : RequestModel + { + public ConsolidationRequest() + { + this.AccountName = WalletManager.DefaultAccount; + } + + [Required(ErrorMessage = "The name of the wallet is missing.")] + public string WalletName { get; set; } + + /// + /// The account from which UTXOs should be consolidated. + /// If this is not set the default account of the selected wallet will be used. + /// + public string AccountName { get; set; } + + [Required(ErrorMessage = "A password is required.")] + public string WalletPassword { get; set; } + + /// + /// If this is set, only UTXOs within this wallet address will be consolidated. + /// If it is not set, all the UTXOs within the selected account will be consolidated. + /// + public string SingleAddress { get; set; } + + /// + /// Which address the UTXOs should be sent to. It does not have to be within the wallet. + /// If it is not provided the UTXOs will be consolidated to an unused address within the specified wallet. + /// + public string DestinationAddress { get; set; } + + /// + /// If provided, UTXOs that are larger in value will not be consolidated. + /// Dust UTXOs will not be consolidated regardless of their value, so there is an implicit lower bound as well. + /// + [MoneyFormat(isRequired: false, ErrorMessage = "The amount is not in the correct format.")] + public string UtxoValueThreshold { get; set; } + } } diff --git a/src/Stratis.Bitcoin.Features.Wallet/Services/IWalletService.cs b/src/Stratis.Bitcoin.Features.Wallet/Services/IWalletService.cs index 4b689f4f23..90a50682da 100644 --- a/src/Stratis.Bitcoin.Features.Wallet/Services/IWalletService.cs +++ b/src/Stratis.Bitcoin.Features.Wallet/Services/IWalletService.cs @@ -59,5 +59,8 @@ Task BuildOfflineSignRequest(BuildOfflineSignRequest r Task OfflineSignRequest(OfflineSignRequest request, CancellationToken cancellationToken); + + Task Consolidate(ConsolidationRequest request, + CancellationToken cancellationToken); } } diff --git a/src/Stratis.Bitcoin.Features.Wallet/Services/WalletService.cs b/src/Stratis.Bitcoin.Features.Wallet/Services/WalletService.cs index b9e8fa7a17..276af61df5 100644 --- a/src/Stratis.Bitcoin.Features.Wallet/Services/WalletService.cs +++ b/src/Stratis.Bitcoin.Features.Wallet/Services/WalletService.cs @@ -1382,6 +1382,101 @@ public async Task OfflineSignRequest(OfflineSignReq }, cancellationToken); } + public async Task Consolidate(ConsolidationRequest request, CancellationToken cancellationToken) + { + return await Task.Run(() => + { + var utxos = new List(); + var accountReference = new WalletAccountReference(request.WalletName, request.AccountName); + + if (!string.IsNullOrWhiteSpace(request.SingleAddress)) + { + utxos = this.walletManager.GetSpendableTransactionsInWallet(request.WalletName).Where(u => u.Address.Address == request.SingleAddress || u.Address.Address == request.SingleAddress).OrderBy(u2 => u2.Transaction.Amount).ToList(); + } + else + { + utxos = this.walletManager.GetSpendableTransactionsInAccount(accountReference).OrderBy(u2 => u2.Transaction.Amount).ToList(); + } + + if (utxos.Count == 0) + { + throw new FeatureException(HttpStatusCode.BadRequest, "Failed to locate any unspent outputs to consolidate.", + "Failed to locate any unspent outputs to consolidate."); + } + + if (utxos.Count == 1) + { + throw new FeatureException(HttpStatusCode.BadRequest, "Already consolidated.", + "Already consolidated."); + } + + + if (!string.IsNullOrWhiteSpace(request.UtxoValueThreshold)) + { + var threshold = Money.Parse(request.UtxoValueThreshold); + + utxos = utxos.Where(u => u.Transaction.Amount <= threshold).ToList(); + } + + Script destination; + if (!string.IsNullOrWhiteSpace(request.DestinationAddress)) + { + destination = BitcoinAddress.Create(request.DestinationAddress, this.network).ScriptPubKey; + } + else + { + destination = this.walletManager.GetUnusedAddress(accountReference).ScriptPubKey; + } + + Money totalToSend = Money.Zero; + var outpoints = new List(); + + TransactionBuildContext context = null; + + foreach (var utxo in utxos) + { + totalToSend += utxo.Transaction.Amount; + outpoints.Add(utxo.ToOutPoint()); + + context = new TransactionBuildContext(this.network) + { + AccountReference = accountReference, + AllowOtherInputs = false, + FeeType = FeeType.Medium, + // It is intended that consolidation should result in no change address, so the fee has to be subtracted from the single recipient. + Recipients = new List() { new Recipient() { ScriptPubKey = destination, Amount = totalToSend, SubtractFeeFromAmount = true } }, + SelectedInputs = outpoints, + + Sign = false + }; + + // Note that this is the virtual size taking the witness scale factor of the current network into account, and not the raw byte count. + int size = this.walletTransactionHandler.EstimateSize(context); + + // Leave a bit of an error margin for size estimates that are not completely correct. + if (size > (0.95m * this.network.Consensus.Options.MaxStandardTxWeight)) + break; + } + + // Build the final version of the consolidation transaction. + context = new TransactionBuildContext(this.network) + { + AccountReference = accountReference, + AllowOtherInputs = false, + FeeType = FeeType.Medium, + Recipients = new List() { new Recipient() { ScriptPubKey = destination, Amount = totalToSend, SubtractFeeFromAmount = true } }, + SelectedInputs = outpoints, + WalletPassword = request.WalletPassword, + + Sign = true + }; + + Transaction transaction = this.walletTransactionHandler.BuildTransaction(context); + + return transaction.ToHex(); + }, cancellationToken); + } + private TransactionItemModel FindSimilarReceivedTransactionOutput(List items, TransactionData transaction) { diff --git a/src/Stratis.Bitcoin.Features.Wallet/WalletTransactionHandler.cs b/src/Stratis.Bitcoin.Features.Wallet/WalletTransactionHandler.cs index 8b77ab3c22..112736a284 100644 --- a/src/Stratis.Bitcoin.Features.Wallet/WalletTransactionHandler.cs +++ b/src/Stratis.Bitcoin.Features.Wallet/WalletTransactionHandler.cs @@ -5,7 +5,6 @@ using Microsoft.Extensions.Logging; using NBitcoin; using NBitcoin.Policy; -using Stratis.Bitcoin.Features.Wallet; using Stratis.Bitcoin.Features.Wallet.Interfaces; using Stratis.Bitcoin.Features.Wallet.Services; using Stratis.Bitcoin.Utilities; @@ -216,6 +215,15 @@ public Money EstimateFee(TransactionBuildContext context) return context.TransactionFee; } + public int EstimateSize(TransactionBuildContext context) + { + this.InitializeTransactionBuilder(context); + + Transaction transaction = context.TransactionBuilder.BuildTransaction(false); + + return context.TransactionBuilder.EstimateSize(transaction, true); + } + /// /// Initializes the context transaction builder from information in . /// @@ -403,16 +411,27 @@ protected virtual void AddRecipients(TransactionBuildContext context) if (context.Recipients.Any(a => a.Amount == Money.Zero)) throw new WalletException("No amount specified."); - int totalRecipients = context.Recipients.Count(r => r.SubtractFeeFromAmount); + int totalSubtractingRecipients = context.Recipients.Count(r => r.SubtractFeeFromAmount); - // If we have any recipients that require a fee to be subtracted from the amount, then - // calculate fee and evenly distribute it among all recipients. Any remaining fee should be + // If none of them need the fee subtracted then it's simply a matter of adding the individual recipients to the builder. + if (totalSubtractingRecipients == 0) + { + foreach (Recipient recipient in context.Recipients) + { + context.TransactionBuilder.Send(recipient.ScriptPubKey, recipient.Amount); + } + + return; + } + + // If the transaction fee has been explicitly specified, and we have any recipients that require a fee to be subtracted + // from the amount to be sent, then evenly distribute the chosen fee among all recipients. Any remaining fee should be // subtracted from the first recipient. - if (totalRecipients > 0 && context.TransactionFee != null) + if (context.TransactionFee != null) { Money fee = context.TransactionFee; - long recipientFee = fee.Satoshi / totalRecipients; - long remainingFee = fee.Satoshi % totalRecipients; + long recipientFee = fee.Satoshi / totalSubtractingRecipients; + long remainingFee = fee.Satoshi % totalSubtractingRecipients; for (int i = 0; i < context.Recipients.Count; i++) { @@ -434,9 +453,19 @@ protected virtual void AddRecipients(TransactionBuildContext context) } else { + // This is currently a limitation of the NBitcoin TransactionBuilder. + // The only alternative would possibly be to recompute the output sizes after the AddFee call. + if (totalSubtractingRecipients > 1) + throw new WalletException($"Cannot subtract fee from more than 1 recipient if {nameof(context.TransactionFee)} is not set."); + + // If the transaction fee has not been explicitly specified yet, then the builder needs to assign it later from the wallet fee policy. + // So we just need to indicate to the builder that the fees must be subtracted from the specified recipient. foreach (Recipient recipient in context.Recipients) { context.TransactionBuilder.Send(recipient.ScriptPubKey, recipient.Amount); + + if (recipient.SubtractFeeFromAmount) + context.TransactionBuilder.SubtractFees(); } } }