Custom transactions from JDC removed from mempool during update #868

NonsoAmadi10 · 2024-04-23T18:15:45Z

What does this PR do?

Closes Custom transactions from JDC removed from mempool during update #840
It reads transactions from the jds mempool and the node's mempool, compares them and adds them to the new mempool that is updated. In cases where the txids are unknown it adds the fat transactions to the updated mempool as well.
To prevent an overflow, old fat transactions are cleared before a new minning job is declared

Fi3 · 2024-05-03T10:49:35Z

This PR needs to be rebased on dev

github-actions · 2024-05-10T21:34:54Z

Bencher

Report	Tue, August 13, 2024 at 15:23:32 UTC
Project	Stratum v2 (SRI)
Branch	bug-missing-transaction-mempool
Testbed	sv1

🚨 3 ALERTS: Threshold Boundary Limits exceeded!

Benchmark	Measure (units)	View	Value	Upper Boundary
serialize_deserialize_subscribe	RAM Accesses (accesses)	🚨 (view plot \| view alert)	322.00 (+1.05%)	321.63 (100.11%)
serialize_submit	RAM Accesses (accesses)	🚨 (view plot \| view alert)	330.00 (+1.63%)	329.18 (100.25%)
serialize_subscribe	RAM Accesses (accesses)	🚨 (view plot \| view alert)	160.00 (+3.10%)	159.67 (100.20%)

Click to view all benchmark results

Benchmark	Estimated Cycles	Estimated Cycles Results estimated cycles \| (Δ%)	Estimated Cycles Upper Boundary estimated cycles \| (%)	Instructions	Instructions Results instructions \| (Δ%)	Instructions Upper Boundary instructions \| (%)	L1 Accesses	L1 Accesses Results accesses \| (Δ%)	L1 Accesses Upper Boundary accesses \| (%)	L2 Accesses	L2 Accesses Results accesses \| (Δ%)	L2 Accesses Upper Boundary accesses \| (%)	RAM Accesses	RAM Accesses Results accesses \| (Δ%)	RAM Accesses Upper Boundary accesses \| (%)
get_authorize	✅ (view plot)	8,553.00 (+1.26%)	8,723.15 (98.05%)	✅ (view plot)	3,772.00 (+0.81%)	3,849.46 (97.99%)	✅ (view plot)	5,293.00 (+0.89%)	5,393.29 (98.14%)	✅ (view plot)	8.00 (+1.82%)	10.22 (78.30%)	✅ (view plot)	92.00 (+1.86%)	94.36 (97.50%)
get_submit	✅ (view plot)	95,702.00 (+0.16%)	96,101.36 (99.58%)	✅ (view plot)	59,522.00 (+0.09%)	59,756.95 (99.61%)	✅ (view plot)	85,502.00 (+0.12%)	85,806.89 (99.64%)	✅ (view plot)	45.00 (-17.11%)	62.66 (71.82%)	✅ (view plot)	285.00 (+1.01%)	287.46 (99.14%)
get_subscribe	✅ (view plot)	8,072.00 (+1.18%)	8,253.15 (97.81%)	✅ (view plot)	2,848.00 (+0.58%)	2,934.64 (97.05%)	✅ (view plot)	3,982.00 (+0.65%)	4,093.37 (97.28%)	✅ (view plot)	13.00 (-17.69%)	19.80 (65.64%)	✅ (view plot)	115.00 (+2.10%)	116.72 (98.53%)
serialize_authorize	✅ (view plot)	12,396.00 (+1.47%)	12,500.78 (99.16%)	✅ (view plot)	5,343.00 (+0.57%)	5,420.46 (98.57%)	✅ (view plot)	7,456.00 (+0.63%)	7,556.64 (98.67%)	✅ (view plot)	8.00 (-25.11%)	13.24 (60.42%)	✅ (view plot)	140.00 (+3.07%)	140.47 (99.66%)
serialize_deserialize_authorize	✅ (view plot)	24,614.00 (+0.54%)	24,717.62 (99.58%)	✅ (view plot)	9,950.00 (+0.50%)	10,019.43 (99.31%)	✅ (view plot)	14,049.00 (+0.59%)	14,141.12 (99.35%)	✅ (view plot)	34.00 (-5.97%)	41.41 (82.11%)	✅ (view plot)	297.00 (+0.58%)	298.90 (99.36%)
serialize_deserialize_handle_authorize	✅ (view plot)	30,256.00 (+0.29%)	30,386.51 (99.57%)	✅ (view plot)	12,127.00 (+0.25%)	12,204.46 (99.37%)	✅ (view plot)	17,161.00 (+0.25%)	17,270.57 (99.37%)	✅ (view plot)	64.00 (+8.39%)	64.82 (98.74%)	✅ (view plot)	365.00 (+0.16%)	368.80 (98.97%)
serialize_deserialize_handle_submit	✅ (view plot)	126,625.00 (+0.17%)	126,996.07 (99.71%)	✅ (view plot)	73,307.00 (+0.08%)	73,594.86 (99.61%)	✅ (view plot)	105,090.00 (+0.10%)	105,476.28 (99.63%)	✅ (view plot)	114.00 (-5.28%)	130.56 (87.32%)	✅ (view plot)	599.00 (+0.67%)	599.31 (99.95%)
serialize_deserialize_handle_subscribe	✅ (view plot)	27,562.00 (+0.36%)	27,606.73 (99.84%)	✅ (view plot)	9,650.00 (+0.17%)	9,736.64 (99.11%)	✅ (view plot)	13,647.00 (+0.16%)	13,768.46 (99.12%)	✅ (view plot)	67.00 (+2.23%)	73.28 (91.43%)	✅ (view plot)	388.00 (+0.52%)	388.82 (99.79%)
serialize_deserialize_submit	✅ (view plot)	115,333.00 (+0.22%)	115,617.76 (99.75%)	✅ (view plot)	68,167.00 (+0.17%)	68,379.37 (99.69%)	✅ (view plot)	97,843.00 (+0.20%)	98,117.90 (99.72%)	✅ (view plot)	61.00 (-11.35%)	75.35 (80.96%)	✅ (view plot)	491.00 (+0.56%)	492.35 (99.73%)
serialize_deserialize_subscribe	✅ (view plot)	23,016.00 (+0.59%)	23,104.55 (99.62%)	✅ (view plot)	8,209.00 (+0.26%)	8,292.08 (99.00%)	✅ (view plot)	11,566.00 (+0.30%)	11,675.65 (99.06%)	✅ (view plot)	36.00 (-7.65%)	43.92 (81.96%)	🚨 (view plot \| view alert)	322.00 (+1.05%)	321.63 (100.11%)
serialize_submit	✅ (view plot)	100,113.00 (+0.23%)	100,432.29 (99.68%)	✅ (view plot)	61,566.00 (+0.09%)	61,805.96 (99.61%)	✅ (view plot)	88,348.00 (+0.11%)	88,659.95 (99.65%)	✅ (view plot)	43.00 (-21.50%)	62.92 (68.34%)	🚨 (view plot \| view alert)	330.00 (+1.63%)	329.18 (100.25%)
serialize_subscribe	✅ (view plot)	11,507.00 (+1.59%)	11,593.46 (99.25%)	✅ (view plot)	4,195.00 (+0.40%)	4,281.64 (97.98%)	✅ (view plot)	5,837.00 (+0.39%)	5,952.23 (98.06%)	✅ (view plot)	14.00 (-13.53%)	18.83 (74.35%)	🚨 (view plot \| view alert)	160.00 (+3.10%)	159.67 (100.20%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

github-actions · 2024-05-10T21:34:54Z

Bencher

Report	Tue, August 13, 2024 at 15:23:29 UTC
Project	Stratum v2 (SRI)
Branch	868/merge
Testbed	sv1

🚨 1 ALERT: Threshold Boundary Limit exceeded!

Benchmark	Measure (units)	View	Value	Lower Boundary	Upper Boundary
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle	Latency (nanoseconds (ns))	🚨 (view plot \| view alert)	794.93 (+5.68%)		790.20 (100.60%)

Click to view all benchmark results

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)	Latency Upper Boundary nanoseconds (ns) \| (%)
client-submit-serialize	✅ (view plot)	6,504.00 (-5.03%)	7,384.02 (88.08%)
client-submit-serialize-deserialize	✅ (view plot)	7,400.40 (-4.68%)	8,354.47 (88.58%)
client-submit-serialize-deserialize-handle/client-submit-serialize-deserialize-handle	✅ (view plot)	7,976.70 (-4.29%)	8,883.70 (89.79%)
client-sv1-authorize-serialize-deserialize-handle/client-sv1-authorize-serialize-deserialize-handle	✅ (view plot)	898.73 (+0.05%)	928.06 (96.84%)
client-sv1-authorize-serialize-deserialize/client-sv1-authorize-serialize-deserialize	✅ (view plot)	701.85 (+0.62%)	719.04 (97.61%)
client-sv1-authorize-serialize/client-sv1-authorize-serialize	✅ (view plot)	249.25 (+0.39%)	255.83 (97.43%)
client-sv1-get-authorize/client-sv1-get-authorize	✅ (view plot)	156.12 (-0.71%)	164.19 (95.08%)
client-sv1-get-submit	✅ (view plot)	6,247.20 (-5.66%)	7,145.94 (87.42%)
client-sv1-get-subscribe/client-sv1-get-subscribe	✅ (view plot)	272.85 (-2.11%)	291.68 (93.54%)
client-sv1-subscribe-serialize-deserialize-handle/client-sv1-subscribe-serialize-deserialize-handle	🚨 (view plot \| view alert)	794.93 (+5.68%)	790.20 (100.60%)
client-sv1-subscribe-serialize-deserialize/client-sv1-subscribe-serialize-deserialize	✅ (view plot)	628.74 (+2.34%)	638.00 (98.55%)
client-sv1-subscribe-serialize/client-sv1-subscribe-serialize	✅ (view plot)	203.68 (-1.01%)	218.94 (93.03%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

github-actions · 2024-05-10T21:34:55Z

Bencher

Report	Tue, August 13, 2024 at 15:23:18 UTC
Project	Stratum v2 (SRI)
Branch	bug-missing-transaction-mempool
Testbed	sv2

Click to view all benchmark results

Benchmark	Latency	Latency Results nanoseconds (ns) \| (Δ%)	Latency Upper Boundary nanoseconds (ns) \| (%)
client_sv2_handle_message_common	✅ (view plot)	44.48 (-0.19%)	45.21 (98.39%)
client_sv2_handle_message_mining	✅ (view plot)	73.48 (+0.80%)	79.93 (91.92%)
client_sv2_mining_message_submit_standard	✅ (view plot)	14.66 (+0.08%)	14.69 (99.82%)
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	271.78 (+2.70%)	283.66 (95.81%)
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	591.76 (-1.31%)	665.07 (88.98%)
client_sv2_open_channel	✅ (view plot)	160.79 (-3.23%)	175.37 (91.69%)
client_sv2_open_channel_serialize	✅ (view plot)	274.19 (-2.86%)	295.63 (92.75%)
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	376.71 (-0.61%)	424.67 (88.71%)
client_sv2_setup_connection	✅ (view plot)	160.15 (-2.35%)	174.30 (91.88%)
client_sv2_setup_connection_serialize	✅ (view plot)	444.48 (-5.47%)	507.12 (87.65%)
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	979.19 (-0.01%)	1,096.71 (89.28%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

github-actions · 2024-05-10T21:34:58Z

Bencher

Report	Tue, August 13, 2024 at 15:23:15 UTC
Project	Stratum v2 (SRI)
Branch	bug-missing-transaction-mempool
Testbed	sv2

🚨 2 ALERTS: Threshold Boundary Limits exceeded!

Benchmark	Measure (units)	View	Value	Lower Boundary	Upper Boundary
client_sv2_handle_message_common	Estimated Cycles (estimated cycles)	🚨 (view plot \| view alert)	2,187.00 (+6.27%)		2,146.34 (101.89%)
client_sv2_handle_message_common	RAM Accesses (accesses)	🚨 (view plot \| view alert)	40.00 (+8.72%)		38.86 (102.93%)

Click to view all benchmark results

Benchmark	Estimated Cycles	Estimated Cycles Results estimated cycles \| (Δ%)	Estimated Cycles Upper Boundary estimated cycles \| (%)	Instructions	Instructions Results instructions \| (Δ%)	Instructions Upper Boundary instructions \| (%)	L1 Accesses	L1 Accesses Results accesses \| (Δ%)	L1 Accesses Upper Boundary accesses \| (%)	L2 Accesses	L2 Accesses Results accesses \| (Δ%)	L2 Accesses Upper Boundary accesses \| (%)	RAM Accesses	RAM Accesses Results accesses \| (Δ%)	RAM Accesses Upper Boundary accesses \| (%)
client_sv2_handle_message_common	🚨 (view plot \| view alert)	2,187.00 (+6.27%)	2,146.34 (101.89%)	✅ (view plot)	473.00 (+0.27%)	488.76 (96.78%)	✅ (view plot)	727.00 (-0.82%)	757.52 (95.97%)	✅ (view plot)	12.00 (+60.69%)	12.03 (99.72%)	🚨 (view plot \| view alert)	40.00 (+8.72%)	38.86 (102.93%)
client_sv2_handle_message_mining	✅ (view plot)	8,235.00 (+0.42%)	8,331.53 (98.84%)	✅ (view plot)	2,143.00 (+0.67%)	2,170.54 (98.73%)	✅ (view plot)	3,170.00 (+0.74%)	3,214.30 (98.62%)	✅ (view plot)	33.00 (-14.36%)	43.60 (75.69%)	✅ (view plot)	140.00 (+0.80%)	141.81 (98.72%)
client_sv2_mining_message_submit_standard	✅ (view plot)	6,366.00 (+1.37%)	6,388.96 (99.64%)	✅ (view plot)	1,756.00 (+0.32%)	1,765.70 (99.45%)	✅ (view plot)	2,556.00 (+0.07%)	2,577.15 (99.18%)	✅ (view plot)	20.00 (+7.55%)	26.22 (76.27%)	✅ (view plot)	106.00 (+2.13%)	106.76 (99.29%)
client_sv2_mining_message_submit_standard_serialize	✅ (view plot)	14,877.00 (+0.75%)	15,018.92 (99.06%)	✅ (view plot)	4,700.00 (+0.12%)	4,709.70 (99.79%)	✅ (view plot)	6,757.00 (+0.03%)	6,777.65 (99.70%)	✅ (view plot)	49.00 (+1.61%)	55.03 (89.05%)	✅ (view plot)	225.00 (+1.36%)	229.62 (97.99%)
client_sv2_mining_message_submit_standard_serialize_deserialize	✅ (view plot)	27,663.00 (+0.66%)	27,828.58 (99.40%)	✅ (view plot)	10,591.00 (+0.39%)	10,591.04 (100.00%)	✅ (view plot)	15,403.00 (+0.35%)	15,407.27 (99.97%)	✅ (view plot)	86.00 (+1.89%)	90.86 (94.65%)	✅ (view plot)	338.00 (+1.02%)	344.57 (98.09%)
client_sv2_open_channel	✅ (view plot)	4,349.00 (-2.93%)	4,610.99 (94.32%)	✅ (view plot)	1,461.00 (0.00%)	1,476.60 (98.94%)	✅ (view plot)	2,159.00 (+0.21%)	2,177.70 (99.14%)	✅ (view plot)	11.00 (-6.96%)	15.01 (73.28%)	✅ (view plot)	61.00 (-5.80%)	68.29 (89.33%)
client_sv2_open_channel_serialize	✅ (view plot)	14,042.00 (-1.04%)	14,457.14 (97.13%)	✅ (view plot)	5,064.00 (0.00%)	5,079.60 (99.69%)	✅ (view plot)	7,327.00 (+0.10%)	7,343.35 (99.78%)	✅ (view plot)	34.00 (-9.95%)	43.43 (78.28%)	✅ (view plot)	187.00 (-2.04%)	198.98 (93.98%)
client_sv2_open_channel_serialize_deserialize	✅ (view plot)	22,689.00 (+0.25%)	22,993.90 (98.67%)	✅ (view plot)	8,027.00 (+0.45%)	8,032.33 (99.93%)	✅ (view plot)	11,669.00 (+0.39%)	11,679.49 (99.91%)	✅ (view plot)	83.00 (+11.02%)	85.80 (96.73%)	✅ (view plot)	303.00 (-0.29%)	314.26 (96.42%)
client_sv2_setup_connection	✅ (view plot)	4,715.00 (+0.41%)	4,761.52 (99.02%)	✅ (view plot)	1,502.00 (0.00%)	1,517.60 (98.97%)	✅ (view plot)	2,275.00 (-0.11%)	2,301.50 (98.85%)	✅ (view plot)	12.00 (+22.17%)	15.55 (77.19%)	✅ (view plot)	68.00 (+0.45%)	69.67 (97.60%)
client_sv2_setup_connection_serialize	✅ (view plot)	16,134.00 (-0.57%)	16,521.97 (97.65%)	✅ (view plot)	5,963.00 (0.00%)	5,978.60 (99.74%)	✅ (view plot)	8,659.00 (+0.02%)	8,682.44 (99.73%)	✅ (view plot)	46.00 (+1.68%)	50.47 (91.14%)	✅ (view plot)	207.00 (-1.34%)	218.80 (94.61%)
client_sv2_setup_connection_serialize_deserialize	✅ (view plot)	35,572.00 (+0.14%)	35,734.46 (99.55%)	✅ (view plot)	14,855.00 (+0.25%)	14,860.18 (99.97%)	✅ (view plot)	21,817.00 (+0.26%)	21,818.83 (99.99%)	✅ (view plot)	98.00 (-2.75%)	114.68 (85.45%)	✅ (view plot)	379.00 (+0.05%)	385.16 (98.40%)

Bencher - Continuous Benchmarking
View Public Perf Page
Docs | Repo | Chat | Help

NonsoAmadi10 · 2024-05-10T21:36:26Z

@lorbax I have made the required changes, I welcome your thoughts

lorbax

Need rebase and change the code logic

roles/jd-server/src/lib/job_declarator/message_handler.rs

roles/jd-server/src/lib/mempool/mod.rs

plebhash · 2024-06-04T15:16:30Z

@NonsoAmadi10 would you be willing to work on #955 as well?

we are trying to implement a practice in the SV2 development community where every bug coming from protocol edge cases needs to be mapped into MG tests

that is strategically important for the robustness of SRI

here's a link for a Github Discussion on this topic: #944

we have some video tutorials teaching how to write MG tests:

NonsoAmadi10 · 2024-06-04T15:35:53Z

@NonsoAmadi10 would you be willing to work on #955 as well?

we are trying to implement a practice in the SV2 development community where every bug coming from protocol edge cases needs to be mapped into MG tests

that is strategically important for the robustness of SRI

here's a link for a Github Discussion on this topic: #944

we have some video tutorials teaching how to write MG tests:

https://www.youtube.com/playlist?list=PL4okm1s0ZpT9YveJBiU51gaaOmNJviugi

https://www.youtube.com/playlist?list=PL4okm1s0ZpT8gWyM0UNKNbFoy5-djIheg

Sure I would love to take a go at it. Sign me up!! Lol

plebhash

CI seems broken on rustfmt

please do the following

run:

cargo +nightly fmt

on all crates included in the CI errors, commit and push again

NonsoAmadi10 · 2024-06-04T16:06:08Z

I have formatted now @plebhash

lorbax

Needs some minor fixes

roles/jd-server/src/lib/job_declarator/message_handler.rs

roles/jd-server/src/lib/mempool/mod.rs

roles/jd-server/src/lib/job_declarator/message_handler.rs

rrybarczyk · 2024-07-17T13:23:42Z

Please update title to follow the commit guidelines.

NonsoAmadi10 · 2024-07-17T14:49:06Z

Please update title to follow the commit guidelines.

@rrybarczyk i don't quite understand.

GitGab19 · 2024-07-17T14:55:43Z

I think she was referring to this document which is linked in our CONTRIBUTING.md: https://cbea.ms/git-commit/

In this specific case, the PR title should be named in a way it tries to wrap the fix you did, instead of just using the same name of the issue.

NonsoAmadi10 · 2024-07-17T15:27:11Z

@rrybarczyk @GitGab19 I have done that and even squashed my commits. Please review

roles/jd-server/src/lib/job_declarator/message_handler.rs

jbesraa · 2024-07-18T15:20:12Z

roles/jd-server/src/lib/job_declarator/message_handler.rs

+    for short_id in transactions_to_remove {
+        let result = mempool.safe_lock(|mempool_| -> Result<(), Error> {
+            // Try to manage this unwrap, we use .ok_or() method to return the proper error
+            let short_ids_map = mempool_


do we need to call mempool.safe_lock in each iteration? couldnt we retrieve short_ids_map before the for and just use it?

This was a feedback from lorban I implemented and modified. If you have a better approach I can compare that

@jbesraa is right
the better approach is to get short_ids_map once for all outside the loop, namely

let short_ids_map = mempool.safe_lock(|a| a.to_short_ids(nonce).unwrap()).unwrap();

right before the

for short_id in transactions_to_remove { ... }

for cycle. If you do not succeed in a reasonable time ask help to me or @jbesraa

That means I would still need to handle the double unwraps well? @lorbax

yes, but I suggest first to write a the code as easy as possible, thest it, then add error handling and then test it deeply. You should check that the code you introduced does what it is supposed to do. In particular, the old job must be removed from jds mempool and while updating the jds mempool the fat transactions are not discarded.
You can do that using testnet4, or a custom signet

I'm worried that to_short_ids is very expensive not sure if is really better to lock the mutex for all that time. Btw we should have a benchmark for cases where there is contention for the lock.

this should look into the new received DeclareMiningJob and remove only the txs that are in the old one but not in the new one. Also better would be to have a rc for each txs and decrement it everytime that the tx is in the old one and not in the new one (this cause we have multiple downstreams). And when we reach 0 we remove the tx

somthing like that: pub mempool: HashMap<Txid, Option<(Transaction,u64)>>

this should look into the new received DeclareMiningJob and remove only the txs that are in the old one but not in the new one. Also better would be to have a rc for each txs and decrement it everytime that the tx is in the old one and not in the new one (this cause we have multiple downstreams). And when we reach 0 we remove the tx

I implemented this suggestion here plebhash@d5ec026

somthing like that: pub mempool: HashMap<Txid, Option<(Transaction,u64)>>

Could we take advantage of the token or other property in JobDeclaratorDownstream in order to do that?

roles/jd-server/src/lib/job_declarator/message_handler.rs

roles/jd-server/src/lib/mempool/mod.rs

jbesraa · 2024-07-19T09:45:55Z

@NonsoAmadi10 could you please rebase and update the branch? Ill help with the outstanding issues

plebhash · 2024-08-03T04:31:45Z

hey @NonsoAmadi10 what's the status here?

do you need any input or help unblocking anything?

NonsoAmadi10 · 2024-08-03T13:20:47Z

hey @NonsoAmadi10 what's the status here?

do you need any input or help unblocking anything?

@plebhash , @jbesraa told me to rebase and update the branch that he would help with outstanding issues. I think that was why I halted. Other than that I intended on closing the opened conversation here. I have time this month to work here

plebhash · 2024-08-05T13:34:17Z

hey @NonsoAmadi10 what's the status here?
do you need any input or help unblocking anything?

@plebhash , @jbesraa told me to rebase and update the branch that he would help with outstanding issues. I think that was why I halted. Other than that I intended on closing the opened conversation here. I have time this month to work here

thanks for the update!

@jbesraa is busy with other activities, so I'll help you with this PR

I'll take a couple of days to study the entire PR history and then I'll get back to you

..to `TryFrom<(u8, &'a mut [u8])> for PoolMessages` Add droppable methods for buffer pool and codec bug: clear fat transactions from mempool before new mining jobs are declared rebase and change the code logic get the list of ids of declared transactions from the old declared_mining_job, and remove exactly those from the mempool chore: format using nightly chore: add a clear_mempool_transaction function chore: format files fetch tx data and insert both key and data if key does not exist in jds mempool modify functions based on PR feedback format file chore: refactor cleared mining jobs format file chore: restructure the jds mempool

Modify the update_mempool function to update the jds mempool to save transactions declared by the downstream and get rid of others. Create a clear_declared_mining_job to remove old fat transactions before a new mining job is declared. Manage every unwrap by returning the proper error class and error message

Shourya742

Hey @NonsoAmadi10 , I see the comments have been marked as resolved, but the requested changes don't seem to be in place. Could you please check?

Shourya742 · 2024-08-13T06:43:54Z

roles/jd-server/src/lib/job_declarator/message_handler.rs

+                    match mempool_.mempool.remove(&txid) {
+                        Some(transaction) => {
+                            debug!("Fat transaction {:?} in job with request id {:?} removed from mempool", transaction, mining_job.request_id);
+                            info!("Fat transaction {:?} in job with request id {:?} removed from mempool", txid, mining_job.request_id);


Can you remove this info log ?

okay, I will

roles/jd-server/src/lib/job_declarator/message_handler.rs

@jbesraa

- Move mempool lock outside the loop to reduce contention - Compare old and new mining jobs to avoid unnecessary removals - Modify transaction counters in-place for better efficiency - Remove unnecessary cloning and HashMap operations - Improve error handling and logging - Address feedback from @jbesraa, @lorbax, @plebhash and @Fi3

NonsoAmadi10 · 2024-08-13T15:16:29Z

@plebhash @lorbax @jbesraa @Fi3 @Shourya742. I have taken the feedback from previous unresolved conversations and a huge help from @plebhash commit. Here is what I did:

Optimize clear_declared_mining_job function

Move mempool lock outside the loop to reduce contention
Compare old and new mining jobs to avoid unnecessary removals( as @fi suggested)
Modify transaction counters in-place for better efficiency(thank you @plebhash)
Remove unnecessary cloning and HashMap operations
Improve error handling and logging

I don't know if what I did addressed it or if it has introduced added complexity. I would welcome your feedback

rrybarczyk · 2024-09-04T13:36:03Z

Thank you so much for the work here, but right now our plan is to refactor the roles completely and we want to avoid large roles PR's as it will eventually be refactored again. We will work on this once we have the proper integration test framework to test it in #1138.

NonsoAmadi10 changed the title ~~WIP: Bug missing transaction mempool~~ WIP: Custom transactions from JDC removed from mempool during update Apr 23, 2024

Fi3 deleted the branch stratum-mining:main May 3, 2024 09:37

Fi3 closed this May 3, 2024

Fi3 reopened this May 3, 2024

NonsoAmadi10 force-pushed the bug-missing-transaction-mempool branch 2 times, most recently from 815f09a to 409a8e6 Compare May 10, 2024 21:33

NonsoAmadi10 marked this pull request as ready for review May 10, 2024 21:33

NonsoAmadi10 changed the title ~~WIP: Custom transactions from JDC removed from mempool during update~~ Custom transactions from JDC removed from mempool during update May 13, 2024

lorbax requested changes May 28, 2024

View reviewed changes

roles/jd-server/src/lib/job_declarator/message_handler.rs Outdated Show resolved Hide resolved

roles/jd-server/src/lib/job_declarator/message_handler.rs Outdated Show resolved Hide resolved

roles/jd-server/src/lib/mempool/mod.rs Show resolved Hide resolved

NonsoAmadi10 force-pushed the bug-missing-transaction-mempool branch from 409a8e6 to 61d617d Compare June 4, 2024 14:44

NonsoAmadi10 requested a review from lorbax June 4, 2024 14:44

plebhash linked an issue Jun 4, 2024 that may be closed by this pull request

Custom transactions from JDC removed from mempool during update #840

Closed

plebhash mentioned this pull request Jun 4, 2024

MG tests reproducing #840 #955

Open

plebhash requested changes Jun 4, 2024

View reviewed changes

NonsoAmadi10 requested a review from plebhash June 4, 2024 16:06

NonsoAmadi10 mentioned this pull request Jun 4, 2024

Handling Orphaned Transactions from Superseded DeclareMiningJob Requests #956

Open

lorbax reviewed Jun 5, 2024

View reviewed changes

roles/jd-server/src/lib/job_declarator/message_handler.rs Outdated Show resolved Hide resolved

roles/jd-server/src/lib/job_declarator/message_handler.rs Outdated Show resolved Hide resolved

NonsoAmadi10 force-pushed the bug-missing-transaction-mempool branch from 3678e84 to 2a05c03 Compare June 6, 2024 14:53

NonsoAmadi10 requested a review from lorbax June 6, 2024 14:54

NonsoAmadi10 force-pushed the bug-missing-transaction-mempool branch from 2a05c03 to 695b8bf Compare June 6, 2024 14:55

lorbax force-pushed the bug-missing-transaction-mempool branch from 2c25b3d to 5870671 Compare June 17, 2024 18:16

lorbax requested changes Jul 14, 2024

View reviewed changes

roles/jd-server/src/lib/job_declarator/message_handler.rs Show resolved Hide resolved

roles/jd-server/src/lib/mempool/mod.rs Show resolved Hide resolved

NonsoAmadi10 requested a review from lorbax July 15, 2024 11:58

lorbax requested changes Jul 15, 2024

View reviewed changes

roles/jd-server/src/lib/job_declarator/message_handler.rs Outdated Show resolved Hide resolved

roles/jd-server/src/lib/job_declarator/message_handler.rs Outdated Show resolved Hide resolved

NonsoAmadi10 requested a review from lorbax July 16, 2024 14:51

NonsoAmadi10 force-pushed the bug-missing-transaction-mempool branch 2 times, most recently from 4700da5 to 1ef967b Compare July 17, 2024 15:26

jbesraa suggested changes Jul 18, 2024

View reviewed changes

NonsoAmadi10 requested a review from jbesraa July 19, 2024 20:42

jbesraa and others added 5 commits August 9, 2024 13:15

chore: format files

4a0200d

chore: modify update_mempool function

1a0a239

chore: remove redundant brackets

5c7d0e3

NonsoAmadi10 force-pushed the bug-missing-transaction-mempool branch from 2ed981c to 5c7d0e3 Compare August 9, 2024 12:17

Shourya742 suggested changes Aug 13, 2024

View reviewed changes

plebhash mentioned this pull request Aug 13, 2024

issues lacking CI #1077

Open

GitGab19 mentioned this pull request Aug 22, 2024

SRI most important, but NOT critical bugs #1138

Open

4 tasks

plebhash changed the base branch from dev to main August 28, 2024 20:18

rrybarczyk closed this Sep 4, 2024

Custom transactions from JDC removed from mempool during update #868

Custom transactions from JDC removed from mempool during update #868

Conversation

NonsoAmadi10 commented Apr 23, 2024 • edited Loading

What does this PR do?

Fi3 commented May 3, 2024

github-actions bot commented May 10, 2024 • edited Loading

Bencher

github-actions bot commented May 10, 2024 • edited Loading

Bencher

github-actions bot commented May 10, 2024 • edited Loading

Bencher

github-actions bot commented May 10, 2024 • edited Loading

Bencher

NonsoAmadi10 commented May 10, 2024

lorbax left a comment

Choose a reason for hiding this comment

plebhash commented Jun 4, 2024 • edited Loading

NonsoAmadi10 commented Jun 4, 2024

plebhash left a comment

Choose a reason for hiding this comment

NonsoAmadi10 commented Jun 4, 2024

lorbax left a comment

Choose a reason for hiding this comment

rrybarczyk commented Jul 17, 2024

NonsoAmadi10 commented Jul 17, 2024

GitGab19 commented Jul 17, 2024

NonsoAmadi10 commented Jul 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fi3 Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

plebhash Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

jbesraa Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

jbesraa commented Jul 19, 2024

plebhash commented Aug 3, 2024

NonsoAmadi10 commented Aug 3, 2024

plebhash commented Aug 5, 2024

Shourya742 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NonsoAmadi10 commented Aug 13, 2024

rrybarczyk commented Sep 4, 2024

NonsoAmadi10 commented Apr 23, 2024 •

edited

Loading

github-actions bot commented May 10, 2024 •

edited

Loading

github-actions bot commented May 10, 2024 •

edited

Loading

github-actions bot commented May 10, 2024 •

edited

Loading

github-actions bot commented May 10, 2024 •

edited

Loading

plebhash commented Jun 4, 2024 •

edited

Loading

Fi3 Jul 23, 2024 •

edited

Loading

plebhash Aug 13, 2024 •

edited

Loading

jbesraa Aug 13, 2024 •

edited

Loading