Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UX/UI] Add CPFP #3395

Merged
merged 2 commits into from
Feb 10, 2022
Merged

[UX/UI] Add CPFP #3395

merged 2 commits into from
Feb 10, 2022

Conversation

NicolasDorier
Copy link
Member

@NicolasDorier NicolasDorier commented Jan 31, 2022

Address: #2509

This add basic CPFP to the invoice list and the wallet transaction list.

In the invoice's list, we assume CPFP is used on Bitcoin. Thus, CPFP in the invoice list doesn't support altcoins.

To use it, select transactions/invoices you want to bump, then click on Bump fee. This will redirect you to the signing flow.
I plan to include an UI intermediary step later before the signing flow.

CPFP will create a new transaction spending the output of all selected invoices/transactions.

The fee rate horizon of this new transaction is currently hardcoded: We assume the user wants this transaction confirmed in the next block.
On top of the CPFP fee, we add fee to all the bumped transactions.
To know how much to bump, we assume the bumped transactions have a fee rate equal to what is needed to be included 20 blocks, and add the difference to what is needed to be included in the next block.

NBXplorer doesn't have the fee rate of transactions, this is why we assume rather than taking the actual fee rate of the bumped transactions.

For example:

  • Network fee rate next block: 12 sat/b
  • In 20 blocks: 2 sat/b

Now imagine we have 2 transaction

  • tx A of real fee rate of 5 sat/b and size of 150 bytes
  • tx B of real fee rate of 10 sat /b and size of 124 bytes.

Now we CPFP both of them with tx C.

The fee of tx C will be (size 160 bytes):

  • The fee paying for C itself, assuming we want to go in next block: 160 * 12 = 1920
  • The fee paying to bump A and B, assuming we want to go in next block (150 + 124) * 12 - (150 + 124) * 2 = 2740

Total = 1920 + 2740 = 4660

I believe we can improve this in the future. Since the transactions are still unconfirmed, it should be possible to get the real fee rate of A and B (5 and 10 sat/b), rather than assuming they both paid 2 sat/b.

Screenshot:
image

image

@NicolasDorier NicolasDorier marked this pull request as draft January 31, 2022 07:24
@NicolasDorier NicolasDorier changed the title Add CPFP [UI] Add CPFP Jan 31, 2022
@NicolasDorier NicolasDorier changed the title [UI] Add CPFP [UX/UI] Add CPFP Jan 31, 2022
@pavlenex
Copy link
Contributor

pavlenex commented Feb 4, 2022

@NicolasDorier Getting this exception thrown:

System.InvalidOperationException: No call to TransactionBuilder.Send has been done which can support the fees
   at NBitcoin.TransactionBuilder.SubtractFees()
   at NBitcoin.TransactionBuilder.SendAll(Script scriptPubKey)
   at NBitcoin.TransactionBuilder.SendAll(IDestination destination)
   at BTCPayServer.Controllers.UIWalletsController.WalletCPFP(WalletId walletId, String[] outpoints, String[] transactionHashes) in /Users/p/Downloads/Guide/btcpayserver/BTCPayServer/Controllers/UIWalletsController.PSBT.cs:line 116
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeInnerFilterAsync>g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextExceptionFilterAsync>g__Awaited|26_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ExceptionContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|25_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeFilterPipelineAsync>g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authorization.Policy.AuthorizationMiddlewareResultHandler.HandleAsync(RequestDelegate next, HttpContext context, AuthorizationPolicy policy, PolicyAuthorizationResult authorizeResult)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at BTCPayServer.Hosting.BTCPayMiddleware.Invoke(HttpContext httpContext) in /Users/p/Downloads/Guide/btcpayserver/BTCPayServer/Hosting/BTCpayMiddleware.cs:line 99
   at BTCPayServer.Hosting.GreenfieldMiddleware.Invoke(HttpContext httpContext) in /Users/p/Downloads/Guide/btcpayserver/BTCPayServer/Hosting/GreenfieldMiddleware.cs:line 30
   at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
   at BTCPayServer.Hosting.HeadersOverrideMiddleware.Invoke(HttpContext httpContext) in /Users/p/Downloads/Guide/btcpayserver/BTCPayServer/Hosting/HeadersOverrideMiddleware.cs:line 29
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

This happens because in UI I selected already confirmed transaction and tried to bump it. Not sure if we can address this in a UI or you maybe need to throw a proper error : You cannot bump a fee to an already confirmed transaction something like that?

Also do you think we can use specific label to label a transction that's bumped? Maybe that can be nice addition?

@NicolasDorier
Copy link
Member Author

@pavlenex fixed the exception

@NicolasDorier
Copy link
Member Author

Ping @dstrukt what icon should I put for that?

@NicolasDorier
Copy link
Member Author

This PR also refactor the signing process to be able to redirect to the right page after the transaction has been broadcasted.
Before this, we would always get redirected to the wallet's transaction list. But if we bumped fee from the invoice list, we want to be back there once it's done.

@dennisreimann
Copy link
Member

Nice, looking forward to incorporate this into #3441.

@dennisreimann
Copy link
Member

Ping @dstrukt what icon should I put for that?

fyi: I've rebased #3441 to include CPFP and removed the archive icon from the invoices actions dropdown menu, as we aren't using icons in the dropdowns anymore. So no need for a new icon here.

@dstrukt
Copy link
Member

dstrukt commented Feb 10, 2022

As echoed in MM, support this decision.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants