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
Bitcoin Core 0.18 RPC #550
Conversation
I don't think it currently solves #423, and would you mind linking to the release notes that mention this new RPC? |
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18PsbtRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18PsbtRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/config/BitcoindInstance.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/serializers/JsonReaders.scala
Outdated
Show resolved
Hide resolved
testkit/src/main/scala/org/bitcoins/testkit/rpc/BitcoindRpcTestUtil.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18PsbtRpc.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, look through the CI logs after they are done running and you can verify that your implementation doesn't have compiler errors 👍
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/BitcoindV18RpcClient.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18PsbtRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/serializers/JsonSerializers.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great if you could open and save BitcoindInstance.scala
, RpcPsbtResult.scala
, JsonSerializers.scala
and BitcoindRpcTestUtil.scala
. We will hopefully end up with a diff that's more readable:-)
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18PsbtRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18AssortedRpc.scala
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18AssortedRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18AssortedRpc.scala
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
it should "utxoUpdateResult contains the input" in { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't really test the functionality of utxoupdatepsbt
. From looking at the docs, it looks like it takes in a PSBT, and updates it with information found in our node. This is probably a bit hard to test, so I'd just leave this out for now, and we can come back to it later.
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18PsbtRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18PsbtRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18AssortedRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/serializers/JsonSerializers.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/serializers/JsonSerializers.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/BitcoindV18RpcClientTest.scala
Outdated
Show resolved
Hide resolved
9bd89c8 Optimize secp256k1_fe_normalize_weak calls. Move secp256k1_fe_normalize_weak calls out of ECMULT_TABLE_GET_GE and ECMULT_TABLE_GET_GE_STORAGE and into secp256k1_ge_globalz_set_table_gej instead. (Russell O'Connor) Pull request description: Move secp256k1_fe_normalize_weak calls out of ECMULT_TABLE_GET_GE and ECMULT_TABLE_GET_GE_STORAGE and into secp256k1_ge_globalz_set_table_gej instead. Tree-SHA512: 7bbb1aca8e37a268a26d7061bd1f390db129e697792f1d5ddd10ea34927616edc26ef118b500c3e5e14d1d463196033ef64e4d34b765380325c24835458b7a9b
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/BitcoindV18RpcClientTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/BitcoindV18RpcClientTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/BitcoindV18RpcClientTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/BitcoindV18RpcClientTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/BitcoindV18RpcClientTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18DescriptorRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18PsbtRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/jsonmodels/RpcPsbtResult.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/serializers/JsonSerializers.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18DescriptorRpc.scala
Outdated
Show resolved
Hide resolved
fb1664f
to
122dec2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because you added a field to Peer
you have to change some JSON readers. You'll have to add this in JsonSerializers
:
implicit val satsPerKbReads: Reads[SatoshisPerKiloByte] =
new Reads[SatoshisPerKiloByte] {
def reads(json: JsValue): JsResult[SatoshisPerKiloByte] =
SerializerUtil.processJsNumber(num =>
SatoshisPerKiloByte(Satoshis(Int64(num.toBigInt()))))(json)
}
And then change peerReads
to:
implicit val peerReads: Reads[Peer] = Json.reads[Peer]
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18AssortedRpc.scala
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18AssortedRpc.scala
Outdated
Show resolved
Hide resolved
984d8bf does not compile for me
|
private def getNodeAddresses( | ||
count: Option[Int]): Future[Vector[GetNodeAddressesResult]] = { | ||
bitcoindCall[Vector[GetNodeAddressesResult]]("getnodeaddresses", | ||
List(Json.toJson(count))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the None
case this turns into "null"
, is this what you want? Or should you just implement the two public getNodeAddresses
calls individually?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Torkel suggested I write it this way to have all possible cases I believe. If the two calls individually work better I can implement that
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18DescriptorRpc.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/client/v18/V18PsbtRpc.scala
Outdated
Show resolved
Hide resolved
* @see [[https://bitcoincore.org/en/doc/0.18.0/rpc/rawtransactions/utxoupdatepsbt/]] | ||
*/ | ||
|
||
def analyzePsbt(psbt: String): Future[AnalyzePsbtResult] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could be wrong but I was under the impression that @torkelrogstad implemented a type for PSBTs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment applies for the two defs below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He did not implement a type for PSBTs it was in a list of his todo's he wanted to add but never got to it. I believe there is an open issue for it right now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/BitcoindV18RpcClientTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
} | ||
} | ||
|
||
//Todo: figure out how to implement a test here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
Attempted to pull down and run f027c23 this morning and these were my error messages
|
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/BitcoindV18RpcClientTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/BitcoindV18RpcClientTest.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/PsbtRpcTest.scala
Outdated
Show resolved
Hide resolved
} | ||
} | ||
it should "correctly analyze a psbt " in { | ||
val psbt = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this from? can you link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ran analyzepsbt on it and got the information from my console to make sure we were parsing the correct numbers from the Json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @Christewart is referring to the actual PSBT and where you got that from, like was it from a doc some place or did you generate it or?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the link at the top for BIP 174, that has all my test vectors included. I will post the description of it so it is easy to search.
it should "analyze a PSBT and return a non-empty result" in { | ||
//PSBT with one P2PKH input and one P2SH-P2WPKH input both with non-final scriptSigs. P2SH-P2WPKH input's redeemScript is available. Outputs filled. | ||
|
||
val psbt = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this pulled from some where? Can you link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link is at the top of the file. Do you think I should add the link to more places? All the test vectors I used are contained in that link. I'll change the link to hyperlink specifically to test vectors.
* @see [[https://bitcoincore.org/en/doc/0.18.0/rpc/rawtransactions/utxoupdatepsbt/]] | ||
*/ | ||
|
||
def analyzePsbt(psbt: String): Future[AnalyzePsbtResult] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/serializers/JsonSerializers.scala
Outdated
Show resolved
Hide resolved
bitcoind-rpc/src/main/scala/org/bitcoins/rpc/serializers/JsonSerializers.scala
Outdated
Show resolved
Hide resolved
testkit/src/main/scala/org/bitcoins/testkit/rpc/BitcoindRpcTestUtil.scala
Outdated
Show resolved
Hide resolved
|
||
joinedF.map { result => | ||
assert(result.contains( | ||
"cHNidP8BAP0LAQIAAAADJoFxNx7f8oXpN63upLN7eAAMBWbLs61kZBcTykIXG/YAAAAAAP7")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should probably assert that it contains at least one more to make the test a little more robust
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that a joined PSBT doesn't necessarily contain both inside of it. It uses parts of each to create a brand new PSBT and its not exactly clear how I can pattern search.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotchya, in that case can you manually run this join and find out what is in it and assert on at least one thing that is contributed from each of the inputs?
Looks like you still have tests failing https://travis-ci.org/bitcoin-s/bitcoin-s/jobs/577071987#L1213 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests pass locally! 🎉
But you need to rebase. Probably a good idea to squash first, seeing as this is 56 commits. I can help you with the rebase process if you'd like, I think it's going to be a somewhat hairy process.
5b17be6
to
0c84c72
Compare
72f78c6
to
5868227
Compare
1dbc639
to
4494c2c
Compare
…7/bitcoin-s into 2019_06_24_AnalyzePSBT
bitcoind-rpc-test/src/test/scala/org/bitcoins/rpc/v18/BitcoindV18RpcClientTest.scala
Show resolved
Hide resolved
} | ||
} | ||
it should "correctly analyze a psbt " in { | ||
val psbt = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think @Christewart is referring to the actual PSBT and where you got that from, like was it from a doc some place or did you generate it or?
|
||
joinedF.map { result => | ||
assert(result.contains( | ||
"cHNidP8BAP0LAQIAAAADJoFxNx7f8oXpN63upLN7eAAMBWbLs61kZBcTykIXG/YAAAAAAP7")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotchya, in that case can you manually run this join and find out what is in it and assert on at least one thing that is contributed from each of the inputs?
testkit/src/main/scala/org/bitcoins/testkit/rpc/BitcoindRpcTestUtil.scala
Outdated
Show resolved
Hide resolved
testkit/src/main/scala/org/bitcoins/testkit/rpc/BitcoindRpcTestUtil.scala
Outdated
Show resolved
Hide resolved
testkit/src/main/scala/org/bitcoins/testkit/rpc/BitcoindRpcTestUtil.scala
Outdated
Show resolved
Hide resolved
…7/bitcoin-s into 2019_06_24_AnalyzePSBT
RPC for Bitcoin Core 0.18.0
All new RPCs have been implemented for V18 currently working on changing deprecated calls currently in the code base. There are tests for most of the RPCs I might be missing 1 or 2 of the node/network RPCs as those are a bit tricky to test. Need to implement newly enumerated warning messages as mentioned in https://github.com/bitcoin/bitcoin/blob/eb7daf4d600eeb631427c018a984a77a34aca66e/src/rpc/protocol.h#L32
Release notes:
https://bitcoincore.org/en/releases/0.18.0/
Update (8/26)
This PR addresses all V18 RPC calls and 18.1 has been considered when writing. There are also organizational things in the code base to make testing and things like that possible. As of now all tests are passing except
getnodeaddresses
and technicallyupdateutxo
as both of those require 2 nodes to be spun up and currently that requiresgeneratetoaddress
but we currently usegenerate
. In a new PR I will fix the usage of generate to make to usable for future releases as it is now removed so moving forward it will be rather important we have it in the code base. Also within the new PR once I have implemented that I will fix thegetnodeaddresses
andupdateutxo
test so they are more useful/will pass. Current state is that when testing with CI V16 or V17 will fail a few random tests because of changes implemented for V18 for example (getpeerinfo
) but with otherwise pass everything else.#734 closes this