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

Manual CPFP #413

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Manual CPFP #413

wants to merge 1 commit into from

Conversation

Zshan0
Copy link
Collaborator

@Zshan0 Zshan0 commented Jun 4, 2022

This PR is to maintain code corresponding to the RFC that is being discussed for the addition of manual CPFP command as a JSONRPC API in revaultd. RFC is present here. #411 is the corresponding milestone tracker

@Zshan0 Zshan0 force-pushed the jsonrpc_test branch 6 times, most recently from 64efaf3 to cdc6c11 Compare June 11, 2022 09:31
@Zshan0 Zshan0 force-pushed the jsonrpc_test branch 3 times, most recently from 645a767 to d4c0832 Compare June 18, 2022 05:44
@Zshan0
Copy link
Collaborator Author

Zshan0 commented Jun 18, 2022

A simple test for cpfp has been written, I'm not sure what other kinds of tests are required. Please let me know

@Zshan0 Zshan0 marked this pull request as ready for review June 18, 2022 13:22
@Zshan0 Zshan0 force-pushed the jsonrpc_test branch 4 times, most recently from 39be8bd to 1d90a44 Compare June 25, 2022 12:12
@Zshan0
Copy link
Collaborator Author

Zshan0 commented Jun 25, 2022

I am unable to figure out a simple way of testing CPFPing multiple SpendTransactions, if there is a simpler scheme that I can follow please let me know.

The Current CPFP is able to terminate invalid txid with an RpcError but it fails to terminate for txid that is valid but is unknown, rather than relying on direct database functions to fetch the given txid and then throw an error if it is unable to find, is there a function that is specifically designed to check if a txid exists in the database that I can use for sanity checking before passing it further?

src/bitcoind/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

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

I had a look at the test for manual CPFP of multiple Spend transactions, which looks valid but cluttered.

tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
@Zshan0 Zshan0 force-pushed the jsonrpc_test branch 3 times, most recently from 446283c to f217739 Compare July 3, 2022 11:18
@Zshan0 Zshan0 force-pushed the jsonrpc_test branch 2 times, most recently from 2df277a to aa275ff Compare July 18, 2022 11:14
@darosior
Copy link
Member

Is this ready for review?

@Zshan0 Zshan0 mentioned this pull request Jul 22, 2022
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

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

Just some comments on the implementation for now. I'll review the tests tomorrow.

Why do you update the tests/servers/ submodules?

doc/API.md Outdated Show resolved Hide resolved
src/bitcoind/mod.rs Outdated Show resolved Hide resolved
doc/API.md Outdated Show resolved Hide resolved
src/bitcoind/mod.rs Outdated Show resolved Hide resolved
src/bitcoind/mod.rs Outdated Show resolved Hide resolved
src/bitcoind/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

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

The message handler in the bitcoind thread is wrong. It would return which would stop the loop, and obviously, make the program crash on shutdown or any further tentative to use the bitcoind thread (such as for another manual CPFP). I guess the reason you have so many small tests is a workaround for this bug...

tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
src/bitcoind/mod.rs Outdated Show resolved Hide resolved
src/bitcoind/mod.rs Show resolved Hide resolved
src/bitcoind/poller.rs Outdated Show resolved Hide resolved
src/bitcoind/poller.rs Show resolved Hide resolved
src/bitcoind/mod.rs Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Outdated Show resolved Hide resolved
tests/test_rpc.py Show resolved Hide resolved
@Zshan0 Zshan0 force-pushed the jsonrpc_test branch 2 times, most recently from 66048ee to a81036a Compare August 18, 2022 20:11
@Zshan0 Zshan0 force-pushed the jsonrpc_test branch 4 times, most recently from e0ac86a to 6b497b4 Compare August 22, 2022 08:38
@darosior
Copy link
Member

Untested, but i think you can use this to externally fund the CPFP wallet:

diff --git a/tests/test_framework/revault_network.py b/tests/test_framework/revault_network.py
index 0a840f8..8dfa68e 100644
--- a/tests/test_framework/revault_network.py
+++ b/tests/test_framework/revault_network.py
@@ -3,6 +3,7 @@ import logging
 import os
 import random
 
+from bip380.descriptors import Descriptor
 from ephemeral_port_reserve import reserve
 from nacl.public import PrivateKey as Curve25519Private
 from test_framework import serializations
@@ -541,6 +542,13 @@ class RevaultNetwork:
 
         return created_vaults
 
+    def fund_cpfp(self, amount):
+        """Send a coin worth this value to the CPFP descriptor"""
+        der_desc = Descriptor.from_str(str(self.cpfp_desc))
+        der_desc.derive(0)
+        decoded = self.bitcoind.rpc.decodescript(der_desc.script_pubkey.hex())
+        self.bitcoind.rpc.sendtoaddress(decoded["address"], amount)
+
     def secure_vault(self, vault):
         """Make all stakeholders share signatures for all revocation txs"""
         deposit = f"{vault['txid']}:{vault['vout']}"

@darosior
Copy link
Member

This should fix the unit tests CI, and also fixes the configuration of the functional tests CI (thanks for reporting it!): #423.

Comment on lines +319 to +326
match tx.cpfp_txin(cpfp_descriptor, &revaultd.secp_ctx) {
Some(txin) => txins.push(txin),
None => {
log::error!("No CPFP txin for tx '{}'", tx.txid());
return Ok(txids.into_iter().collect());
}
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rather than returning all the transactions, I need to throw an error here. I'm not sure which one

Copy link
Member

Choose a reason for hiding this comment

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

You can create a new enum variant for it if none other fit.

Comment on lines +1350 to +1359
def get_unvault_txids(wallet, vaults):
unvault_txids = []
for vault in vaults:
deposit = f"{vault['txid']}:{vault['vout']}"
unvault_psbt = serializations.PSBT()
unvault_b64 = wallet.rpc.listpresignedtransactions([deposit])[
"presigned_transactions"
][0]["unvault"]
unvault_psbt.deserialize(unvault_b64)
unvault_psbt.tx.calc_sha256()
unvault_txids.append(unvault_psbt.tx.hash)
return unvault_txids
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it a good practice to import functions from other test files? This function is present in test_misc.py. I can also move it into a different file. Let me know what would be the best approach

Copy link
Member

Choose a reason for hiding this comment

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

I would make it a method of the RevaultNetwork class.

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.

2 participants