bitcoin-teleport / teleport-transactions

CoinSwap implementation
Other
233 stars 62 forks source link

teleport attempts to use jsonrpc 2.0 despite it not being supported by Core #43

Open Kixunil opened 2 years ago

Kixunil commented 2 years ago

After unsuccessful attempt to try this out I recorded the communication and it looks like jsonrpc 2.0 is the problem:

Recorded request from teleport:

POST /wallet/teleport HTTP/1.1
Host: localhost:18441
Content-Type: application/json
Authorization: Basic X19jb29raWVfXzowMzM4Nzc1MTMyOGVmOTJiNGEzOGIxNDg2MDY5NmNkNzg3YWQ2ZWNkNWNiYmE1NGFjOTI2YzM2YmQwMjYwNThm
Content-Length: 65

{"method":"getblockchaininfo","params":[],"id":1,"jsonrpc":"2.0"}

Request from bitcoin-cli:

POST / HTTP/1.1
Host: 127.0.0.1
Connection: close
Content-Type: application/json
Authorization: Basic X19jb29raWVfXzowMzM4Nzc1MTMyOGVmOTJiNGEzOGIxNDg2MDY5NmNkNzg3YWQ2ZWNkNWNiYmE1NGFjOTI2YzM2YmQwMjYwNThm
Content-Length: 50

{"method":"getblockchaininfo","params":[],"id":1}

(don't worry about the "leak", it's a disposable VM :))

I'm still confused how come this wasn't discovered already...

P.S.: thanks a lot for working on this project!!!

chris-belcher commented 2 years ago

Thanks for the issue.

At first glance it's pretty strange to me. I've been successfully using the project for ages, and I've used Bitcoin Core 0.21 and 22.0. Before release I did cargo clean and rebuilt the project from scratch, and made some coinswaps on regtest and signet, so I'm also confused how something like this could've been missed. There must be some small detail...

I'll think a little some more.

edit: fwiw teleport doesn't implement its own json-rpc, it uses https://github.com/rust-bitcoin/rust-bitcoincore-rpc

Kixunil commented 2 years ago

Do we have same dependencies? Cargo.lock is not committed (it should be for binaries), so maybe I have something different. Possibly relevant:

name = "jsonrpc"
version = "0.11.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "436f3455a8a4e9c7b14de9f1206198ee5d0bdc2db1b560339d2141093d7dd389"
chris-belcher commented 2 years ago

Looks like I have the same.

[[package]]
name = "jsonrpc"
version = "0.11.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "436f3455a8a4e9c7b14de9f1206198ee5d0bdc2db1b560339d2141093d7dd389"
dependencies = [
 "hyper 0.10.16",
 "serde",
 "serde_derive",
 "serde_json",
]

But good thinking, I'm pretty sure I have kept the same Cargo.lock this whole time, I've renamed it and am now building from scratch to see if I reproduce the error.

edit: attached is my whole file Cargo.lock.txt

chris-belcher commented 2 years ago

I just tried running teleport after cargo clean and deleting the Cargo.lock file. And it worked fine.

In case its helpful heres the new Cargo.lock file Cargo-new.lock.txt

I'm sure you've already checked this, but do you have a wallet called teleport loaded in Bitcoin Core? It has to be an old-style wallet not a descriptor wallet. Bitcoin Core 22.0 creates descriptor wallets by default now so watch out (Indeed one of the TODOs is to move teleport to using descriptor wallets, since they are the future clearly)

Kixunil commented 2 years ago

Interesting, will investigate. I think wallets shouldn't affect getblockchaininfo RPC, right?

chris-belcher commented 2 years ago

You're right they shouldnt.

run-watchtower is a subroutine which does not use wallets at all, so that could be a useful test maybe.

Kixunil commented 2 years ago
$ diff Cargo.lock Cargo-new.lock.txt 
2a3,4
> version = 3
> 

Weird. Maybe Rust version is relevant? I have 1.59. Also, I used debug mode.

Kixunil commented 2 years ago

I tried to bump dependencies: bitcoin to 0.27.1 and bitcoincore-rpc to 0.14 and it seem to have helped. I can send a PR during the day (UTC+1) if you want. It's trivial anyway.

chris-belcher commented 2 years ago

I intentionally downgraded because of this issue: https://github.com/rust-bitcoin/rust-bitcoincore-rpc/issues/211

Are you able to reproduce that issue on your end? Try creating a new wallet, it should run importmulti

Kixunil commented 2 years ago

I tried the code at the issue you posted as well as running generate-wallet of teleport-transactions (with a new, empty wallet) and both succeeded.

chris-belcher commented 2 years ago

Oh weird.

I guess then update it on your end and we'll leave the current versions on the repos, when more people try out the software we can see how best to solve it then.

Kixunil commented 2 years ago

TBH, I'm starting to be afraid of UB since the behavior is so non-deterministic. Hopefully I can run it through asan/ubsan when I get a bit of time.

chris-belcher commented 2 years ago

What is UB?

Kixunil commented 2 years ago

Undefined behavior. Of course it'd mean a problem in some unsafe block in one of the dependencies.