coordinator: get_spend_transaction #31
Conversation
edouardparis
force-pushed
the
get-spend-transaction
branch
6 times, most recently
from
46314d6
to
5173723
Compare
edouardparis
force-pushed
the
get-spend-transaction
branch
from
6f081b8
to
253b3e8
Compare
There was a problem hiding this comment.
I still need to review the tests, but i already have a number of comments so i'll push that. A small meta point: you put a lot of thoughts into this PR so it would have been nice to convey that to its reviewers (or future archeologists) by giving some insights/rationale in the OP or the commit messages. :)
I think we should definitely experiment more on the plugin side. The current interface has the advantage of being simple. But it puts the burden on the plugin to handle the de-duplication. Is it ok? I don't know, off-hand i'd say yes but we should try before committing to it.
I think having a test with a whitelist plugin and a many-Unvaults Spend with the Unvaults in different blocks would be good to have.
| @@ -114,7 +116,40 @@ def bitcoind(directory): | |||
|
|
|||
|
|
|||
| @pytest.fixture | |||
| def miradord(request, bitcoind, directory): | |||
| def noise_keys(): | |||
| /// Get Spend transaction spending the vault with the given deposit outpoint. | ||
| /// Beware that the spend transaction may be invalid and needs to be verified against | ||
| /// libbitcoinconsensus. |
There was a problem hiding this comment.
What is the purpose of checking the Spend transaction against libbitcoinconsensus?
There was a problem hiding this comment.
I may have misunderstood libbitcoinconsensus usage, but it was to check that the sigs and the tx are valid and can pass the btc contensus and that the coordinator did not trick us with a fake spend that will never pass the mempool
There was a problem hiding this comment.
libbitcoinconsensus defines part of the consensus (and even more so of the standard Bitcoin Core relay policy). It can only do context-less checks. For instance, it would not be able to detect if the inputs of the Spend don't exist.
But if they committed to an invalid Spend.. It's their problem? It will be unspendable through the manager path so no harm can be done (assuming cosigning servers aren't compromised, which Spend things always do). They will have to Cancel all the Unvaults.
edouardparis
force-pushed
the
get-spend-transaction
branch
from
253b3e8
to
cc23359
Compare
edouardparis
force-pushed
the
get-spend-transaction
branch
from
fc9442a
to
2898996
Compare
| | `value` | integer | Value of the vault in satoshis | | ||
| | `deposit_outpoint` | string | Deposit outpoint of the vault | | ||
| | `unvault_tx` | string | Psbt of the unvault transaction of the vault | | ||
| | `candidate_tx` | string or null | Hex encoded transaction spending the vault, null if the watchtower did not retrieve it from coordinator | |
edouardparis
force-pushed
the
get-spend-transaction
branch
from
669a235
to
75d7793
Compare
edouardparis
force-pushed
the
get-spend-transaction
branch
from
75d7793
to
e819759
Compare
This PR introduce:
A new module
coordinatorthat fetch spend transaction for each vault unvaulting.The coordinator configuration is made optional. In case of no configuration, the watchtower does not fetch the spend transaction for each spending attempt.
A spend transaction fetched from the coordinator is checked agains libbitcoinconsensus. It prevents the managers colliding to sign an "evil" spend and sign it with cosigners and share a "legit" spend with no signatures to watchtowers to bypass spending policies.
A new plugin:
whitelist, that revault vaults which candidate spend transaction contains recipient script pubkey not present in the authorized list. Beware that the change address and the cpfp address of the spend must be part of the whitelist too in order for the spend to go through (This behavior maybe changed in the future)Fixes #23.