updates to newer jsonrpc dependencies containing swappable transports #180
Conversation
|
The build only seems to fail on Rust 1.29 Is it necessary/desirable for me to adjust changes to be 1.29 compatible? |
For now, yes. Our MSRV might change later this year,please see rust-bitcoin/rust-bitcoin#510 for more details. |
|
Oh, sorry, I didn't even look at the problem and assumed it was just an issue with the code, but it seems the fault of our build system, let me check … |
|
It looks like theres a bunch of danging dependency pins happening in the contrib script for 1.29. I'm zeroing in on now, but if you know the answer please feel free to update the PR. |
|
So, the problem is that some dependencies change their MSRV in minor releases but that's a breaking change for us. So we need to pin these dependencies. Over time more and more did so and e.g. the log dependency exists in two different major versions making pinning even harder because now I need to specify which one I want to pin (that's why I did the weird parsing with |
|
The newest commit I have seems to have solved the 1.29 issue. I'll let the rest of the build play out and see if I broke anything else in the process. |
|
OK. I've finally reached a point where the source itself is failing here. It looks like the raw value calls were not in serde_json at the pinned dependency height. The question is, can I update the pinned dependency, or am I gonna have to hack the code to have conditional compilation of the code in question. |
|
You can try to increase the version, but the probability for the pinning-reason still existing is quite high. So you might end up with conditional compilation or just bite the bullet and use |
|
Is Travis CI down? |
|
Maybe they are doing brown-outs to prepare us for them shutting down? Idk, need to investigate, we should probably move to GH actions anyway. Maybe you could try amend+force pushing later. |
|
OK it looks like I've satisfied the build. Should I squash all my commits down to one? |
ProofOfKeags
force-pushed
the
dependencies/jsonrpc
branch
from
e8aadbd
to
34bde5e
Compare
|
Can I get some eyes on this now? @sgeisler |
There was a problem hiding this comment.
Looks mostly good, but idk if we want to do it like that now or think about the design of jsonrpc again @apoelstra.
Minor nit: please don't mix formatting changes with actual ones, but I think it's still clear enough here.
I'm quite happy to see that apparently we can remove some version pinning without everything breaking 😃
|
I am happy to fix small things about the PR however I'd strongly request that "rethinking the design" doesn't prevent this from getting merged as there are several upstream projects that need to be able to plug into Bitcoin Core nodes that are hosted on .onion addresses. This is the least invasive change required to be able to support those use cases as best I can tell. |
|
I agree, that sounds like an important feature, I wasn't aware of that. |
There was a problem hiding this comment.
Not great, not terrible. I'd much rather re-engineer the whole API, but Tor support is important for downstream users, and this PR enables that. So I think it's worth merging and cutting a new release and worrying about overengineering it later. What do you think @stevenroose?
|
This looks fine to me. The choice between |
|
Sooooo can we merge it then? |
|
Bump. |
|
I think this is good to go, other than the nit abuot @stevenroose @sgeisler can I merge this? |
… used due to rust 1.29 compatibility requirements
|
All clear from my side! |
|
Is @stevenroose required to merge this? It seems he is not going to respond. |
|
Ideally yes, but he was very absent so I'll just merge it now. At some point we'd also need to cut a release, there we'll need him anyway afaik. |
|
Has anyone heard from him recently? This change holds up bwt's ability to support tor nodes. |
This is similar to #154 but it seems that has stalled. I fixed the "temporary hack" found in #154 as well.
I ran the integration tests against regtest and everything seems to work. Seems like we should get this in so that we can create a tor transport.
My question separately from this PR is where should the work for adding tor transports go? Should it be part of the actual jsonrpc library? Should it be part of this package? or should it be its own package entirely?