Skip to content

multi-Cancel revaultd #393

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
Merged
darosior merged 14 commits into from
May 16, 2022
Merged

multi-Cancel revaultd #393

darosior merged 14 commits into from
May 16, 2022

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