Skip to content

Comments

multi-Cancel revaultd#393

Merged
darosior merged 14 commits intorevault:masterfrom
darosior:feebumping_riddance
May 16, 2022
Merged

multi-Cancel revaultd#393
darosior merged 14 commits intorevault:masterfrom
darosior:feebumping_riddance

Conversation

@darosior
Copy link
Member

@darosior darosior commented May 2, 2022 •
edited
Loading

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.

@darosior darosior force-pushed the feebumping_riddance branch from b17feb4 to 5af1075 Compare May 2, 2022 13:00
@darosior
Copy link
Member Author

darosior commented May 2, 2022 •
edited
Loading

Still TODO: Done

  • Remove db_cancel_transaction()
  • Check for remaining single-cancel helpers in db/interface

@darosior darosior force-pushed the feebumping_riddance branch 2 times, most recently from 07c233e to 2e2a02b Compare May 3, 2022 07:30
@darosior
Copy link
Member Author

darosior commented May 5, 2022 •
edited
Loading

Note to self: we'll need to update revaultd's master to net and tx master before merging this. I made the small change for the accept() modification part of the first commit

@darosior darosior force-pushed the feebumping_riddance branch from c2f5d24 to ff30e56 Compare May 5, 2022 14:13
@darosior
Copy link
Member Author

darosior commented May 5, 2022

Rebased on #394

@darosior darosior force-pushed the feebumping_riddance branch 3 times, most recently from a2fc9dc to 552df8a Compare May 6, 2022 17:48
@edouardparis edouardparis mentioned this pull request May 9, 2022
Merged
@edouardparis
Copy link
Member

edouardparis commented May 9, 2022

Manually tested with revault/revault-gui#350, seems to be fine

@darosior darosior mentioned this pull request May 10, 2022
Open
@darosior darosior force-pushed the feebumping_riddance branch from 552df8a to e764c4a Compare May 11, 2022 12:13
@darosior
Copy link
Member Author

darosior commented May 11, 2022

Rebased on master

@darosior darosior force-pushed the feebumping_riddance branch 4 times, most recently from 4d68ab4 to cf176ac Compare May 12, 2022 10:18
@darosior darosior force-pushed the feebumping_riddance branch from cf176ac to f5ece6f Compare May 13, 2022 09:45
@darosior
Copy link
Member Author

darosior commented May 13, 2022

Yay, this finally passed CI, i'll re-kick it to be sure it's not flaky.

@darosior darosior changed the title WIP: multi-Cancel revaultd multi-Cancel revaultd May 13, 2022
@darosior darosior requested a review from edouardparis May 13, 2022 15:23
darosior added 6 commits May 13, 2022 18:57
@darosior
qa: update miradord and cosignerd to the new message serialization
175478b
@darosior
commands: update [get,set]revocationtxs flow for all Cancel transactions
5caa118
@darosior
commands: remove the unit test of listpresigned txs
ae2839d 
It's not that useful as listpresignedtxs is already exercised under all
conditions by the functional tests. And it would be a MAJOR pain to move
it to the multi-cancel logic. As the usefulness / time spent ratio is
very low, just drop it.

I'd still like to add it back if possible, but definitely not with
hardcoded PSBTs.
@darosior
commands: adapt the result of 'listpresignedtxs' to multiple Cancel txs
9465c35
@darosior
db: remove the db_cancel_transaction routine
32a7ec5 
It was specific to single-Cancel logic
@darosior
Adapt the doc single-cancel wording to multi-cancel
ae94b9a
@darosior darosior force-pushed the feebumping_riddance branch from f5ece6f to ae94b9a Compare May 13, 2022 16:57
@darosior
Copy link
Member Author

darosior commented May 13, 2022

Updated the miradord submodule to master.

edouardparis
edouardparis reviewed May 14, 2022
View reviewed changes
src/commands/utils.rs
}

/// Get the Unvault transaction for a given vault
pub fn unvault_tx(
Copy link
Member

@edouardparis edouardparis May 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, in the future we should move these helpers in a scripts or consensus module with a struct handling the descriptors and the secp_ctx, encapsulating the derivation etc. Revaultd would store this struct and we use it like this:
revaultd.consensus.unvault_tx(&vault.derivation_index) or revaultd.generator.unvault_tx(&derivation_index)
(I dont know yet which name)

edouardparis
edouardparis approved these changes May 14, 2022
View reviewed changes
Copy link
Member

@edouardparis edouardparis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK ae94b9a
Manually tested with the GUI

@darosior darosior merged commit 6cb5913 into revault:master May 16, 2022
edouardparis added a commit to revault/revault-gui that referenced this pull request May 16, 2022
@edouardparis
Merge #350: app + dummysigner: handle multi cancel derivation and sig…
f433bbc 
…nature

5314664 dummysigner: bump revault_tx and change api (edouard)
712a102 app: handle multiple cancel txs (edouard)
ae1cccd bump revaultd (edouard)

Pull request description:

  based on revault/revaultd#393

ACKs for top commit:
  edouardparis:
    ACK 5314664

Tree-SHA512: 30adc9198a8faf7ff0c13e9b0ca714a23252c847c93b5718cb916366eed1745ad1366575ba2624a1833ee0029d1e79f83d3ba937f1d9756fc234e19986de2930
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edouardparis edouardparis edouardparis approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@darosior @edouardparis