Skip to content

Jds mempool enhancement 3#16

Merged
GitGab19 merged 5 commits into
GitGab19:jds-mempool-enhancementfrom
lorbax:jds-mempool-enhancement-3
Mar 21, 2024
Merged

Jds mempool enhancement 3#16
GitGab19 merged 5 commits into
GitGab19:jds-mempool-enhancementfrom
lorbax:jds-mempool-enhancement-3

Conversation

@lorbax
Copy link
Copy Markdown

@lorbax lorbax commented Mar 20, 2024

apply part of the suggestions to this PR
stratum-mining#772

lorbax added 2 commits March 19, 2024 10:53
As suggested in
stratum-mining#772 (comment)
A task that before was blocking now is moved as a separate task on main
As suggested in
stratum-mining#772 (comment)
This commit introduces a verification that all the transactions in a job
are correctly recognized when a submit block message appears.
Comment thread roles/jd-server/src/lib/mempool/mod.rs Outdated
// retrieved from the jd client
for txid in txids {
let transaction = client
.get_raw_transaction(&txid.to_string(), None)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

before fetching txs you should check if they are already in the mempool

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

another option could be to use an HashSet right after the receiver

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I am struggling to figure out a useful case for HashSet. If you propose to use it instead HashMap, then you cant store transactions only by their IDs. What do you mean with "right after the receiver"? Can you be a bit more specific? ^^

BTW already fixed in this commit

Error management when some transactions are missing. The jds should not
break. Instead, tries to recover it by triggering the jds mempool to
retrieve the transactions from its bitcoin node
@lorbax lorbax force-pushed the jds-mempool-enhancement-3 branch from 1833c24 to 2d564a1 Compare March 20, 2024 17:56
lorbax added 2 commits March 21, 2024 09:41
In the message handler the message the triggers the JDS mempool to fill
the transactions is implemented in a bad way. Now it is fixed

Add some documentation
// the trasnactions sent to the mempool can be freed
let _ = self_mutex.safe_lock(|a| {
a.add_txs_to_mempool.add_txs_to_mempool_inner = AddTrasactionsToMempoolInner {
known_transactions: vec![],
Copy link
Copy Markdown

@Fi3 Fi3 Mar 21, 2024

Choose a reason for hiding this comment

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

better to use clear, no need to deallocate if we are going to fill it again

}
JobDeclaration::SubmitSolution(_) => todo!(),
}
Self::send(self_mutex.clone(), m).await.unwrap();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this always send the message also for the error cases

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

if we get one of the error cases above something really strange is happening better to close the connection. Downstream will decide what to do

Ok(transactions_list)
}

async fn send_txs_to_mempool(self_mutex: Arc<Mutex<Self>>) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think that you can use an &Arc<Mutex> here

Ok(hex::encode(serialize(&block)))
}

fn are_all_job_transactions_present(
Copy link
Copy Markdown

@Fi3 Fi3 Mar 21, 2024

Choose a reason for hiding this comment

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

change the function name i something else, this seems just a condition check but it do collect txs.

you can use colletc_txs_for_job

.unwrap();
let sender_add_txs_to_mempool = add_txs_to_mempool.sender_add_txs_to_mempool;
let add_txs_to_mempool_inner = add_txs_to_mempool.add_txs_to_mempool_inner;
let _ = sender_add_txs_to_mempool
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why you ignore the error here?

.send(add_txs_to_mempool_inner)
.await;
// the trasnactions sent to the mempool can be freed
let _ = self_mutex.safe_lock(|a| {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same

.clone()
.safe_lock(|a| a.mempool.clone())
.unwrap();
tokio::select! {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

seems that you want to use a tokio::timeout here

sender.clone(),
&config,
mempool.clone(),
// each downstream has its own sender (multi producer single consumer)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

remove this comment you see it from the type

@GitGab19 GitGab19 merged commit c2bc80c into GitGab19:jds-mempool-enhancement Mar 21, 2024
GitGab19 pushed a commit that referenced this pull request Mar 21, 2024
GitGab19 pushed a commit that referenced this pull request Mar 21, 2024
NonsoAmadi10 pushed a commit to NonsoAmadi10/stratum that referenced this pull request Mar 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants