-
Notifications
You must be signed in to change notification settings - Fork 114
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
new rpc-client for JD-Server mempool #761
new rpc-client for JD-Server mempool #761
Conversation
ec5aec9
to
7bec385
Compare
Bencher
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
Bencher
Bencher - Continuous Benchmarking View Public Perf Page Docs | Repo | Chat | Help |
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 tested this (on top of #752) against the TP on a custom Signet (using -debug=rpc
).
I saw successful calls to getrawmempool
.
I created a transaction to ensure that getrawtransaction
is called.
I waited for it to find a block, which causes submitblock
to be called (on one of the nodes, the other one got the solution via the SubmitSolution
sv2 message).
I did not look at the code.
7bec385
to
3145739
Compare
why not use |
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.
SRI has been criticized about our lack of discipline in the way we leave TODO
comments on the code
for example: #723 (comment)
I understand the notion of PR atomicity and I fully agree that sometimes there are things that maybe fall out of the scope of some specific PR, so I don't think writing TODO
comments on a PR is a cardinal sin.
However, I believe we can raise the bar on how we manage these kind of practices. My suggestion is: for every TODO
comment, open an issue with a meaningful description, and paste the link to the issue along with the TODO
comment.
That way, future tasks are still manageable in our roadmap and external eyes have a little bit of context on how the code is evolving.
Because it imported the json-rpc library, which in turn imported serde in a way that it is not compatible with the way it is imported ion the SRI. Moreover, import bitcoincore rpc or just using vendored json-rpc is worse from a performance point of view |
Completely agree with that. I must say that the todo that I left are very marginal, by the way. |
af76e26
to
798ec79
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.
Review done.
I left some questions to clarify some doubts I have (especially in the submit_solution_handler
part)
build-on-all-workspaces.sh
Outdated
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 ran it and I got:
Running fmt on: benches
error: toolchain 'nightly-x86_64-apple-darwin' is not installed
Fmt failed in: benches
Why do you use +nightly
?
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.
we need nightly to run cargo fmt
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.
we need nightly to run cargo fmt
Why? I always ran it locally with simply cargo fmt
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 is very strange in the config file we have:
imports_indent = "Block"
imports_layout = "Mixed"
imports_granularity = "Crate"
that use unstable features available only on nighlty
9eb85a8
to
0ee5082
Compare
192f441
to
0d9b165
Compare
fd68321
to
014ad86
Compare
utils/rpc/src/mini_rpc_client.rs
Outdated
// TODO | ||
// - WHEN UPGRADING TO 1.0.1 Client is in hyper-utils: | ||
// Struct hyper_util::client::legacy::Client | ||
// - use https for security reasons |
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 Bitcoin Core RPC does not support https. You should connect on the same machine (or via a secure tunnel). There's work in progress to support unix domain sockets: bitcoin/bitcoin#27375 (for now I think that's limited to p2p, not rpc)
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.
Thank you for pointing this out!
} | ||
let merkle_root = | ||
merkle_root_from_path(&coinbase_pre[..], &coinbase_suf[..], &extranonce[..], &path) | ||
.expect("Invalid coinbase"); |
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.
maybe you should use try_from instead of panicing
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.
you are right but before I must make the code work. I leave this conversation open. When works I will go back and fix 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.
Still open issue
A new bitcoin rpc custom rpc client is added. Before at the end of the trait method implementation handle_submit_solution in jd-server/src/lib/job_declarator/message_handlers.rs there was the part of the code responsible for submitting the block to the bitcoin node via rpc. This was a blocker. Now when submit_solution arrives, it is sent though a channel to the mempool, that submits it to the node. This task is managed asychronically in the main, so that we don't have to wait that the rpc client submits the block to the bitcoincore node. The error management of JdsMempoolError enum is moved in mempool::error module whenever possible in order to keep main clean.
old vendored json-rpc removed last fixes
- moved mini rpc client in /utils - removed unused dependencies in the jds-server - the channel (submit_solution_sender, submit_solution_receiver) = unbounded() is now bounded but there is no management of when the limit is received - new method of JorDeclaretorDownstream (server) that take submit solution and calculates the block hex. This method relies on a new function in roles-logic-sv2::utils::submit_solution_to_block() that calculates the block given the list of transactions, the DeclareMiningJob and the SubmitSolution - better error management in jds-server main
apply suggestions of the second round of review. - removed unnecessary Arc<Mutex<>> - in the handler implementation for the jds, when a submit_block is received, returns a SendTo::None(Some(SubmitSolution)). Accordingly changes on JobDeclaratorDownstream:::Start - close connection if a message SubmitSolution is sent but there is no job declared in the downstream - frunction roles-logic::utils::submit_solution_to_block is sobstituted with the implementation of From on a wrapper struct - removed catch-all arms in pattern match of handle_message_job_declaration in JobDeclaratorDownstream::start - removed explicit dependancy of library bytes, now relies on hyper::body::Bytes (that uses the same library under the hood, btw)
removed comment and /benches/Cargo.lock
5da7bf5
to
0a99214
Compare
Since the task that listen for new blocks is now performed independently in main, if the url rpc config is the jds is left empty the mempool the method `get_client` called in `JDsMemppol::on_submit()` returns `NoClient` error. Since we do not want rpc connections in message generator, the url config is left empty, resulting in a lot of `NoCLient` errors in the JD-server, which led the MG tests involving jd-server to fail. Also changed some variables name to make them more intuitive
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.
addressed this suggestion by @plebhash:
#782
This PR we through three rounds of reviews, that are visible in the commit history
This is a new and rebased version of PR716
What is in this PR.
A new Rpc client, called MiniRpcClient. This client communicates with a bitcoin node, and the requests that can do are just the following: ask for the mempool, ask for a specific transaction, submit a bitcoin block. This client is used to make the jd-server mempool communicate with the bitcoin node. The previous client was depending upon json-rpc library, which caused some issues because imported serde in a way that the SRI "doesn't like". This rpc client is made on top of the latest hyper 1.1.0 library for http connections.
The code is asynchronous and non blocking. Before was "almost" non blocking, as the function "submit_block" that concerned submitting a block to the bitcoin node, was blocking.
I also took advantage of introduce some proper error management in the jds mempool module.
This PR has been tested in regtest and needs to be tested in testnet.