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

Feat/verify mainstay inclusion #78

Conversation

DhananjayPurohit
Copy link
Contributor

No description provided.

@ariard
Copy link
Contributor

ariard commented Nov 8, 2023

see #79 with functional RPC calls on bitcoind

with following config details

# Password for JSON-RPC connections
rpcpassword=hello_world

# Username for JSON-RPC connections
rpcuser=civkitd_client

# Use the chain <chain> (default: main). Allowed values: main, test,
# signet, regtest
chain=regtest

implementing verifytxoutproof passthrough, that way civkit sample should be able to verify proofs by relying on a civkitd node, alternatively.

will review more this one.

@DhananjayPurohit
Copy link
Contributor Author

So, this verifytxoutproof should be called using the command from civkit-cli, right? Also the files having .proto extension need to be modified or its auto-generated?

Cargo.toml Outdated
simplelog = "0.7.1"
dirs = "3.0.1"
log = "0.4.14"
staking_credentials = { git = "https://github.com/civkit/staking-credentials.git", branch = "main" }
reqwest = "0.11.20"
base64 = "0.21.4"
jsonrpc = "0.14.0"
rs_merkle = "1.4.1"
bitcoincore-rpc = "0.17.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove the usage of this crate. Latest branch introduce a functional RPC client, gives us more flexibility on parameters formatting.

@@ -47,8 +51,19 @@ impl BitcoindClient {

}

pub async fn verifytxoutproof() {
pub async fn verifytxoutproof(txid: String, slot: usize, mut inclusion_proof: InclusionProof) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is where we’re out-of-sync:

verifytxoutproof is a RPC method in Bitcoin Core: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/txoutproof.cpp#L123

This method takes a CMerkleBlock and then return a result if the pointed to txid has been included in the chain. CMerkleBlock contains a CPartialMerkleTree (which internally points to a txid).

Here verifytxoutproof should be just a call to self.rpc_client.call(“verifytxoutproof”, &[merkle_block]) to realize the latest step of mainstay proof verification, namely that the txid has been included in the chain.

Now in term of API, we have two options (at least):

  • a) move this verification code (commitment, slot proof, merkle root inclusion) in the client-side (i.e into civkit-sample)
  • b) have just the client with a method verifiyinclusionproof and a given txid

As the ultimate step of the verification flow is dependent on chain access and txidindex (which takes some server ressource), mobile clients will most of the time rely on a server. So I think we can have this code here, just wrapping verify_commitments / verify_slot_proof and verify_merkle_root_inclusion in a dedicated mainstay method and have this in notaryd calling back verifytxoutproof for the latest step).

Choose a reason for hiding this comment

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

Want to clarify what we want to do here and how.

In order to verify the merkle root inclusion in the mainstay transaction we need to get the scriptPubKey of the tx and to verify it's confirmation. The current method is that the mainstay proof just contains the merkle root (and path) and the TxID, and the verification process consists of using getrawtransaction to confirm the txid is confirmed AND get the scriptPubKey from the raw/deserialised tx returned (then verify scriptPubKey is tweaked with the merkle root).

This is the simplest approach as it only means the mainstay 'proof' object needs the TxID and merke root, however it also requires a node with txindex=1.

If we want to enable verification of proofs with a pruned node (with txindex=0) then we can use verifytxoutproof . But for this to work, we need to know the a) The full raw transaction and b) The proof generated by gettxoutproof . These would have to be included in the 'proof' object.

gettxoutproof requires txindex=1 so a a pruned node would only suffice for verification, you would need a full node for generating proofs.

Does this make sense - we should go with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to enable verification of proofs with a pruned node (with txindex=0) then we can use verifytxoutproof . But for this to work, we need to know the a) The full raw transaction and b) The proof generated by gettxoutproof . These would have to be included in the 'proof' object.

I think we’re in sync here - Note to call verifytxoutproof you only need the merkle block generated by gettxoutproof, the full raw transaction isn’t a requirement, see the tests in rpc_txoutproof.py.

That said, of course better to use getrawtransaction if this fits the current usage of verifying the merkle root inclusion in a mainstay transaction. Note you can use getrawtransaction with the current civkitd rpc framework with Client::call().

Note my feedback was mostly on the resource trade-offs and verification responsibility between client and server. As both alternatives are requiring txindex=1, I don’t think it matters. I can see a new verifyinclusionproof with the scriptpubkey, the txid and the merkle root, the commitment and the merkle path. What is unclear to me is how the user learns the scriptpubkey, from our conversation here: #64 (comment)

I think we should go with this, even if the high-level API we should introduce for the civkit user at the client-level (i.e currently civkit-sample is unclear to me, we can address this in a follow-up PR.

Choose a reason for hiding this comment

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

The user can only learn the scriptPubKey from the raw (deserialised) tx - that's why it needs to be included in the proof if can't be retrieved with getrawtransaction .

@ariard
Copy link
Contributor

ariard commented Nov 14, 2023

So, this verifytxoutproof should be called using the command from civkit-cli, right?

This is bottom interface servicing all the civkit components that need themselves access to bitcoind state, so CredentialGateway and mainstay to verify inclusion proof for now (in the future maybe to serve some on-chain payment proof).

Also the files having .proto extension need to be modified or its auto-generated?

The .proto are manually edited and then auto-generated by the tonic framework in rust code which is itself compiled to binary.

Architecturally, I think it could be valuable to start using mainstay proof generation and verification in notaryd, for now just using as a front-end for mainstay server, then we can add more functionalities with time, e.g announcing mainstay as a service over ln gossips or nostr.

@ariard
Copy link
Contributor

ariard commented Nov 22, 2023

Will review more this one, we can add the good API for civkit clients matching the underlying mainstay operations in future PR.

@ariard
Copy link
Contributor

ariard commented Dec 13, 2023

Thanks for the work here - Rather to review when to manually fix the issues, see #109.
You should be credited in the commit message. I’ll land it, we can do follow-up PRs after.

@ariard ariard closed this Dec 13, 2023
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.

3 participants