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.

Sign up for GitHub

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

Jump to bottom

About (not) dying on the dynamic feebumping hill #119

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

About (not) dying on the dynamic feebumping hill #119

wants to merge 6 commits into from

Conversation

darosior
Copy link
Member

Given that in the past 2 years we could not find reasonable workarounds to the pinning vectors of pre-signing revocation transactions with ANYONECANPAY [0] and sponsoring via RBF by adding new inputs [1], releasing the first version of Revault with a mechanism for dynamic fee-bumping would cause more harm than good.
Instead, we presign a set of Cancel transactions in advance at a given distribution of feerates (say, 20sats/vb, 100sats/vb, 200sats/vb, 500sats/vb and 1000sats/vb). For the Emergency, we presign it at an enormous feerate in advance (1000sats/vb) since it's not supposed to be used.

It's conceptually a reduction in security, as if the market feerate gets above 1000sats/vb at any point in time in the future after a v0 deployment of Revault all bets are off regarding the enforceability of the policies or the Emergency deterrent on this deployment (apart if it was reset with new parameters in the meantime).

Another downside is that it prevents the anti fee sniping protection that i projected to implement by using the fee-bumping inputs' nSequence. We now have transactions that pay a lot of fee and can end up on the network without any such protection (since they are presigned you cant set an appropriate timelock).

On the other hand, this helps a lot with some other things:

  • No need to allocate fee bumping reserves, nor the need for the complexity of managing those reserves
  • There was a very slight perverse incentive to wait before broadcasting the Cancel transactions, as its fees were mostly paid from single-owned coins (feebumping reservers). They are now paid from the shared coins.
  • This closes Prevent ANYONECANPAY-signed inputs replay #83, as we don't have this concern anymore with SIGHASH_ALL.

Fixes #114

[0] Mentioned for instance here in the third section.
[1] bitcoin/bitcoin#23121

@kloaec
Copy link
Member

kloaec commented Feb 17, 2022

extra considerations:

  • need to sign multiple "fee versions" of the security txs, so the hardware wallet may take some time to do its job
  • more granularity decreases wasted fees, but increases number of security transactions signatures. Not too much of an issue as security transactions aren't supposed to be needed unless under attack (so wasting some fees is a lesser issue)
  • SO MUCH SIMPLER <3

I feel so sorry for all the work that the team put into the feebumping strategy, WT reserves, optimizations etc, to end up with such a specific attack. Hopefully this work can be reused in the future!

@darosior
Copy link
Member Author

darosior commented Feb 17, 2022 •
edited
Loading

Worth mentioning, covenants (even the most basic ones) would help here. Combined with Taproot you could have N different Cancel transactions for a cost of log(N) in the witness script size and a cost of 0 in added communications.

Actually, you could have Cancel transactions with a feerate up to the value of the vault itself so you don't need dynamic feebumping anymore. But then it allows any party with knowledge of the Script to burn the funds (or if you add a pubkey challenge anyone taking part in the contract). EDIT: actually you could add a timelock. So you can say if after 90% of the timelock expired, burn everything to fees. But then you skew miner incentives.

@JSwambo
Copy link
Member

JSwambo commented Feb 18, 2022

Nice! I just have one suggestion to update introduction.md too. Can you add this to the end of the file:

"Since Cancel TXs may (or may not) occur frequently, it is important to minimise their feerates while still ensuring timely confirmation. The best solution currently is to prepare multiple variants of the same Cancel TX with a broad distribution of feerates, from typical to extreme and with many in-between. At the time of broadcast, the one with the appropriate fee should be selected."

@darosior
Copy link
Member Author

darosior commented Feb 18, 2022 via email

@darosior darosior mentioned this pull request Feb 23, 2022
Merged
@darosior
Copy link
Member Author

darosior commented Mar 16, 2022 •
edited
Loading

TODO:

  • check that we respect the replacement rules for all possible sizes
  • [ ] increase the feerates a bit I don't think it's justified, and it is not specified here.
  • [ ] Apply the message serialization changes There was none actually
  • Update introduction.md

@darosior darosior mentioned this pull request May 2, 2022
Merged
darosior added a commit to revault/revault_tx that referenced this pull request May 6, 2022
@darosior
Merge #122: Dynamic feebumping riddance
23f18b6 
461d83f fuzz: remove tests for >1 inputs cases (Antoine Poinsot)
d7467e5 transactions: don't require the sighash type to be set in the PSBT inputs (Antoine Poinsot)
4029351 transactions: don't require the sighash type flag (Antoine Poinsot)
4e6cd3c transactions: return an Amount in RevaultTransaction's fees() (Antoine Poinsot)
d989744 transactions: multi-feerates Cancel transaction (Antoine Poinsot)
3029ed1 transactions: don't take nLockTime as parameter for presigned transactions (Antoine Poinsot)
ffbcb4e transactions: introduce a RevaultPresignedTransaction trait (Antoine Poinsot)
b85da2f transactions: merge create_psbt for revocation transactions (Antoine Poinsot)
ce83882 transactions: make the revocation_tx_inputs parsing checks common (Antoine Poinsot)
9b3e06c qa: simplify transaction satisfaction (Antoine Poinsot)
880f01b transactions: sign revocation transactions using SIGHASH_ALL (Antoine Poinsot)
9e531ac contrib: fixup the source-based coverage script (Antoine Poinsot)
a8ea8c5 transactions: remove the fee bumping input logic (Antoine Poinsot)
69dfa01 transactions: increase the Emergency tx feerate to 1000sat/vb (Antoine Poinsot)

Pull request description:

  This implements revault/practical-revault#119.

  The conceptual simplification of the transactions management enabled some cleanups and refactorings, which i jumped on right away in this PR.

ACKs for top commit:
  edouardparis:
    ACK 461d83f

Tree-SHA512: 10aed596de123d8a4c7dc8e7cb35926e0c77393cd918f38c2912e3c110e3b4063acda24777a656dfa2e9adcce66a23c5e2eff1a3bfb4f4a16e1e5504a1cc21d2
@darosior darosior mentioned this pull request May 13, 2022
Merged
darosior added a commit to revault/miradord that referenced this pull request May 13, 2022
@darosior
Merge #25: First step toward static feebumping
e589c35 
390bda4 poller: remove the TODO related to the feebumping wallet (Antoine Poinsot)
0062022 poller: choose the Cancel transaction to broadcast based on estimatesmartfee (Antoine Poinsot)
37536ec qa: mockable bitcoind (Antoine Poinsot)
1a1a31b listener: expect and treat all the Cancel transactions (Antoine Poinsot)
2585495 db: store and query the Cancel signatures at various feerates (Antoine Poinsot)
d7b42d4 Update to latest revault_tx and revault_net (Antoine Poinsot)
2339e2f main: try to downcast panics to String, too (Antoine Poinsot)
16c04fe ci: bump MSRV to 1.48 (Antoine Poinsot)

Pull request description:

  This implements revault/practical-revault#119.

  We start by adapting the listener to the new messages, and make it treat incoming Cancel signatures for the various Cancel transactions (where it previously only had to manage for a single one).
  We continue by adapting the poller loop to choose the correct Cancel transaction to broadcast depending on the fee estimates. This is only a first step, more needs to be considered for a proper fee bumping integration.
  Finally, we get rid of the fee bumping wallet mentions: we are not doing that anymore (for the time being).

ACKs for top commit:
  edouardparis:
    utACK 390bda4

Tree-SHA512: a1b3b0abe1fd66b65a51293525ef5aff451edc8690295b74416fc922d3f24ca4cc6fc4513b141c28c892393168644fcea96c25983df20a25ebe62759bf4d8cf8
darosior added a commit to revault/revaultd that referenced this pull request May 16, 2022
@darosior
Merge #393: multi-Cancel revaultd
6cb5913 
ae94b9a Adapt the doc single-cancel wording to multi-cancel (Antoine Poinsot)
32a7ec5 db: remove the db_cancel_transaction routine (Antoine Poinsot)
9465c35 commands: adapt the result of 'listpresignedtxs' to multiple Cancel txs (Antoine Poinsot)
ae2839d commands: remove the unit test of listpresigned txs (Antoine Poinsot)
5caa118 commands: update [get,set]revocationtxs flow for all Cancel transactions (Antoine Poinsot)
175478b qa: update miradord and cosignerd to the new message serialization (Antoine Poinsot)
f5a2162 Share all Cancel transactions' signatures with the watchtowers (Antoine Poinsot)
74c2f1c rpc: over-simplified adaptation of 'revault' command to multi-Cancel (Antoine Poinsot)
8683a03 poller: notice any possible Cancel when an Unvault is spent. (Antoine Poinsot)
a0585c5 poller: adapt Cancel rebroadcast to multi-Cancel (Antoine Poinsot)
30d2d20 poller: adapt Cancel confirmation tracking to multi-Cancel (Antoine Poinsot)
effb3f7 bitcoind: store all the Cancel transactions on vault confirmation (Antoine Poinsot)
b9b1727 db: simplify the unsigned presigned txs storage (Antoine Poinsot)
410a627 Update revault_tx and revault_net (Antoine Poinsot)

Pull request description:

  This implements revault/practical-revault#119.

  We adapt all components to use multi-Cancel:
  - Sharing and fetching the signatures for all the Cancel transactions
  - Monitoring the chain for any of the Cancel transaction
  - Changing the various interfaces to have all Cancel transactions instead of a single one

  Feedback on the new RPC API would be welcome: i preferred a list of Cancel instead of a mapping from feerate to Cancel.
  We still don't chose the Cancel to broadcast depending on the fee estimation in `revault`.

ACKs for top commit:
  edouardparis:
    ACK ae94b9a

Tree-SHA512: cb2e1b5a13f84188be7897a239b045b5a6ac307af8f9843cc56d689f51d50decf5510b1ea52c3832bed5b937f6a788e555c46bf04a2612553c5ba89fcaacfe4f
darosior added 6 commits May 17, 2022 10:57
@darosior
Presign all transactions with SIGHASH_ALL, remove ANYONECANPAY mentions
1636b74 
There are still too many pinning attacks possible, such that it's not
reasonable to deploy with this. In addition, it's certainly not worth
the complexity and overhead of managing fee-bumping reserves.

This is in theory a reduction in security as the maximum reserve is now
a deployment parameter (at what feerate to sign the Cancel txs) where it
was previously flexible and left to the discretion of each watchtower.
@darosior
messages: typo 'inactive'
9d7f246
@darosior
messages: share the Cancel with several feerates to the WTs
7bb40a9 
For the coordinator, we can just use the current `sig` message.
@darosior
transactions: remove the mention of a static presigned feerate for th…
bdf88e9 
…e Cancel
@darosior
transactions: presign the Emergency transaction at 1000sat/vb
603f19f 
Since we got rid of fee-bumping, we need a feerate such that it is
*very* unlikely the next block feerate ever goes above (which'd make the
deterrent somewhat moot).

On the other hand, choosing something much higher really conditions the
deposit size. Especially in case of deposit split.
For 8 stakeholders and 3 managers, this makes the deposit be at least
500€ in order for the Emergency to not burn >25% of the funds at this
feerate. (Emergency vbytes: 211, BTCEUR: 40k.)
@darosior
Update the introduction to mention static fee bumping
a9ecf05
@darosior
Copy link
Member Author

cc @JSwambo, @edouardparis. This was implemented and is now ready for final review before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielabrozzoni danielabrozzoni Awaiting requested review from danielabrozzoni

@edouardparis edouardparis Awaiting requested review from edouardparis

Assignees
No one assigned
Labels
None yet
Projects
Revault project overview
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Getting rid of fee-bumping
3 participants
@darosior @kloaec @JSwambo