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

Add hot wallet #43

Closed
wants to merge 0 commits into from
Closed

Add hot wallet #43

wants to merge 0 commits into from

Conversation

dr-orlovsky
Copy link
Member

No description provided.

Copy link
Contributor

@zoedberg zoedberg left a comment

Choose a reason for hiding this comment

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

cargo build fails in this branch, the compiler says:

error: cannot find attribute `clap` in this scope
   --> src/bip43.rs:120:7
    |
120 |     #[clap(skip)]
    |       ^^^^

error: could not compile `bp-wallet` (lib) due to 1 previous error

As a temporary fix I'm now building with all features enabled, but I'm now encountering the following error:

error: failed to select a version for `unicode-normalization`.
    ... required by package `idna v0.5.0`
    ... which satisfies dependency `idna = "^0.5.0"` (locked to 0.5.0) of package `url v2.5.0`
    ... which satisfies dependency `url = "^2.2"` (locked to 2.5.0) of package `reqwest v0.12.4`
    ... which satisfies dependency `reqwest = "^0.12.4"` (locked to 0.12.4) of package `bp-esplora v0.11.0-beta.6 (/mnt/dmc/zoe/work/bitfinex/rgb-integration-tests/bp-esplora-client)`
    ... which satisfies path dependency `bp-esplora` (locked to 0.11.0-beta.6) of package `rgb-integration-tests v0.1.0 (/mnt/dmc/zoe/work/bitfinex/rgb-integration-tests)`
versions that meet the requirements `^0.1.22` (locked to 0.1.23) are: 0.1.23

all possible versions conflict with previously selected packages.

  previously selected package `unicode-normalization v0.1.22`
    ... which satisfies dependency `unicode-normalization = "=0.1.22"` of package `bip39 v2.0.0`
    ... which satisfies dependency `bip39 = "^2.0.0"` of package `bp-wallet v0.11.0-beta.6.1 (/mnt/dmc/zoe/work/bitfinex/rgb-integration-tests/bp-wallet)`
    ... which satisfies path dependency `bp-wallet` (locked to 0.11.0-beta.6.1) of package `rgb-integration-tests v0.1.0 (/mnt/dmc/zoe/work/bitfinex/rgb-integration-tests)`

failed to select a version for `unicode-normalization` which could resolve this conflict

Not sure how we should fix this, bp-wallet depends on the bip39 crate which depends on unicode-normalization version 0.1.22 while bp-esplora depends on a version of reqwest which indirectly depends on unicode-normalization version 0.1.23

@zoedberg
Copy link
Contributor

zoedberg commented Jul 4, 2024

Temporarily fixed by enabling only the cli feature instead of the all one. I'm now encountering the following error though:

error[E0432]: unresolved import `bpstd::XpubSpec`
  --> rgb/src/descriptor.rs:33:48
   |
33 |     TapTree, Terminal, XOnlyPk, XpubDerivable, XpubSpec,
   |                                                ^^^^^^^^ no `XpubSpec` in the root

warning: impl trait in impl method signature does not match trait method signature
   --> rgb/src/descriptor.rs:195:38
    |
195 |     fn xpubs(&self) -> impl Iterator<Item = &XpubSpec> { iter::once(self.internal_key.xpub_spec()) }
    |                                      ^^^^^^^^^^^^^^^^ this bound is stronger than that defined on the trait
    |
    = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate
    = note: we are soliciting feedback, see issue #121718 <https://github.com/rust-lang/rust/issues/121718> for more information
    = note: `#[warn(refining_impl_trait_reachable)]` on by default
help: replace the return type so that it matches the trait
    |
195 |     fn xpubs(&self) -> impl Iterator<Item = &XpubAccount> { iter::once(self.internal_key.xpub_spec()) }
    |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

warning: impl trait in impl method signature does not match trait method signature
   --> rgb/src/descriptor.rs:299:38
    |
299 |     fn xpubs(&self) -> impl Iterator<Item = &XpubSpec> {
    |                                      ^^^^^^^^^^^^^^^^ this bound is stronger than that defined on the trait
    |
    = note: add `#[allow(refining_impl_trait)]` if it is intended for this to be part of the public API of this crate
    = note: we are soliciting feedback, see issue #121718 <https://github.com/rust-lang/rust/issues/121718> for more information
help: replace the return type so that it matches the trait
    |
299 |     fn xpubs(&self) -> impl Iterator<Item = &XpubAccount> {
    |                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

For more information about this error, try `rustc --explain E0432`.
warning: `rgb-runtime` (lib) generated 2 warnings
error: could not compile `rgb-runtime` (lib) due to 1 previous error; 2 warnings emitted

P.S. All repos are on master except for bp-wallet, bp-std and bp-core that are on the signer branch

@zoedberg
Copy link
Contributor

zoedberg commented Jul 4, 2024

@dr-orlovsky
After fixing rgb-runtime I've managed to try this PR (and the related ones).

Signing of the PSBT seems to work fine but then when trying to deserialize the psbt as so:

let _sig_count = psbt.sign(&signer).unwrap();
let serialized = psbt.serialize(psbt.version);
let tx = bp::Tx::consensus_deserialize(serialized).unwrap();

I receive a panic:
panicked at library/alloc/src/raw_vec.rs:25:5: capacity overflow

Moreover, a couple of questions:

  • could you explain why it's called TestSigner and not just Signer? Could you rename this?
  • could you please remove the lifetime from the TestSigner? It doesn't seem necessary and it complicates its usage a lot.

P.S. I know privately I told you that we needed only taprot signing support but I was wrong, we also need wpkh signing support, sorry.

@dr-orlovsky
Copy link
Member Author

I have rebased the branch on top of master which now contains fixes to the build issues. I have also added commit fixing clap attribute problem you have reported.

Could you explain why it's called TestSigner and not just Signer

Since this signer signs only keys for testnet (and panics on mainnet keys). Maybe TestnetSigner would be a better name, but it is clearly not just a universal Signer. There is another signer, named ConsoleSigner, which can be found in src/hot/signer.rs, which signs mainnet, but (1) I do not think you need one for the tests and (2) it asks user to confirm the receivers, fees etc, since without this the signer is unsafe.

could you please remove the lifetime from the TestSigner?

No, I can't. It takes Xprivs by reference, since Xprivs are not Copy/Clone for the security reasons.

But you can always write your own whatever custom signer, named Signer and taking Xprivs with ownership: just copy file signers.rs from bp-std crate and remove references there. However you will have just a single-use Xpriv keys which can't be used with any other signer. So it would be something like OwningTestnetSigner (similar difference as between str and String, or Path and PathBuf).

I know privately I told you that we needed only taprot signing support but I was wrong, we also need wpkh signing support, sorry.

Good news: Segwit and legacy sighash business logic it's already there (and this is the hardest part of signing). Just need to do one more function in PSBT to support it - will do in the next few days. I am also working on getting the test environment where I can get actual signing happening, so I will be able to debug the panic you have reported.

@dr-orlovsky
Copy link
Member Author

PS. I have updated bp-std and renamed TestSigner into RefTestnetSigner. I also added TestnetSigner which takes ownership over private keys and doesn't use lifetimes.

@zoedberg
Copy link
Contributor

zoedberg commented Jul 5, 2024

Great, thanks! I've opened RGB-WG/rgb#217 to fix the build issues on rgb-runtime and pushed a signer branch on the integration tests where you can run cargo test --test transfers transfer_loop::case_04 to see the panic (remember to first update the git submodules with git submodule update).

@dr-orlovsky
Copy link
Member Author

dr-orlovsky commented Jul 11, 2024

let _sig_count = psbt.sign(&signer).unwrap();
let serialized = psbt.serialize(psbt.version);
let tx = bp::Tx::consensus_deserialize(serialized).unwrap();

I receive a panic: panicked at library/alloc/src/raw_vec.rs:25:5: capacity overflow

Well, of course it won't work! You are deserializing PSBT as a bitcoin transaction, which is not!

You need first to finalize PSBT, exporting signed transaction in such a way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants