Improve coinbase #1248
Improve coinbase #1248
Conversation
Bencher Report
|
| Branch | ImproveCoinbase |
| Testbed | sv1 |
Click to view all benchmark results
| Benchmark | Estimated Cycles | Benchmark Result estimated cycles (Result Δ%) | Upper Boundary estimated cycles (Limit %) | Instructions | Benchmark Result instructions (Result Δ%) | Upper Boundary instructions (Limit %) | L1 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | L2 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | RAM Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| get_authorize | 📈 view plot 🚷 view threshold | 8,482.00 (+0.30%) | 8,677.15 (97.75%) | 📈 view plot 🚷 view threshold | 3,663.00 (-1.87%) | 3,862.95 (94.82%) | 📈 view plot 🚷 view threshold | 5,107.00 (-2.31%) | 5,448.63 (93.73%) | 📈 view plot 🚷 view threshold | 10.00 (+24.27%) | 15.55 (64.31%) | 📈 view plot 🚷 view threshold | 95.00 (+4.27%) | 96.15 (98.80%) |
| get_submit | 📈 view plot 🚷 view threshold | 95,362.00 (-0.02%) | 95,704.61 (99.64%) | 📈 view plot 🚷 view threshold | 59,263.00 (-0.27%) | 59,711.26 (99.25%) | 📈 view plot 🚷 view threshold | 85,072.00 (-0.31%) | 85,824.20 (99.12%) | 📈 view plot 🚷 view threshold | 49.00 (+10.58%) | 59.61 (82.20%) | 📈 view plot 🚷 view threshold | 287.00 (+2.33%) | 290.57 (98.77%) |
| get_subscribe | 📈 view plot 🚷 view threshold | 7,965.00 (-0.50%) | 8,234.92 (96.72%) | 📈 view plot 🚷 view threshold | 2,758.00 (-2.20%) | 2,943.41 (93.70%) | 📈 view plot 🚷 view threshold | 3,830.00 (-2.70%) | 4,143.53 (92.43%) | 📈 view plot 🚷 view threshold | 15.00 (+23.79%) | 20.07 (74.73%) | 📈 view plot 🚷 view threshold | 116.00 (+1.28%) | 117.83 (98.45%) |
| serialize_authorize | 📈 view plot 🚷 view threshold | 12,263.00 (-0.02%) | 12,510.43 (98.02%) | 📈 view plot 🚷 view threshold | 5,240.00 (-1.24%) | 5,427.64 (96.54%) | 📈 view plot 🚷 view threshold | 7,278.00 (-1.54%) | 7,603.72 (95.72%) | 📈 view plot 🚷 view threshold | 10.00 (+1.75%) | 18.34 (54.51%) | 📈 view plot 🚷 view threshold | 141.00 (+2.28%) | 142.35 (99.05%) |
| serialize_deserialize_authorize | 📈 view plot 🚷 view threshold | 24,704.00 (-0.03%) | 25,180.66 (98.11%) | 📈 view plot 🚷 view threshold | 9,786.00 (-0.82%) | 10,021.43 (97.65%) | 📈 view plot 🚷 view threshold | 13,784.00 (-1.01%) | 14,186.24 (97.16%) | 📈 view plot 🚷 view threshold | 35.00 (-1.45%) | 45.46 (76.99%) | 📈 view plot 🚷 view threshold | 307.00 (+1.28%) | 313.26 (98.00%) |
| serialize_deserialize_handle_authorize | 📈 view plot 🚷 view threshold | 30,341.00 (+0.04%) | 30,714.44 (98.78%) | 📈 view plot 🚷 view threshold | 11,989.00 (-0.58%) | 12,194.50 (98.31%) | 📈 view plot 🚷 view threshold | 16,946.00 (-0.75%) | 17,303.05 (97.94%) | 📈 view plot 🚷 view threshold | 61.00 (+9.62%) | 67.11 (90.89%) | 📈 view plot 🚷 view threshold | 374.00 (+0.87%) | 379.20 (98.63%) |
| serialize_deserialize_handle_submit | 📈 view plot 🚷 view threshold | 126,520.00 (+0.05%) | 126,778.20 (99.80%) | 📈 view plot 🚷 view threshold | 73,117.00 (-0.20%) | 73,527.14 (99.44%) | 📈 view plot 🚷 view threshold | 104,760.00 (-0.25%) | 105,494.21 (99.30%) | 📈 view plot 🚷 view threshold | 110.00 (+3.80%) | 124.70 (88.21%) | 📈 view plot 🚷 view threshold | 606.00 (+1.43%) | 608.19 (99.64%) |
| serialize_deserialize_handle_subscribe | 📈 view plot 🚷 view threshold | 27,943.00 (+0.13%) | 28,420.81 (98.32%) | 📈 view plot 🚷 view threshold | 9,577.00 (-0.63%) | 9,757.99 (98.15%) | 📈 view plot 🚷 view threshold | 13,513.00 (-0.83%) | 13,834.08 (97.68%) | 📈 view plot 🚷 view threshold | 72.00 (+12.24%) | 77.36 (93.07%) | 📈 view plot 🚷 view threshold | 402.00 (+0.80%) | 411.04 (97.80%) |
| serialize_deserialize_submit | 📈 view plot 🚷 view threshold | 115,241.00 (-0.00%) | 115,718.60 (99.59%) | 📈 view plot 🚷 view threshold | 67,894.00 (-0.26%) | 68,417.74 (99.23%) | 📈 view plot 🚷 view threshold | 97,356.00 (-0.33%) | 98,281.91 (99.06%) | 📈 view plot 🚷 view threshold | 70.00 (+9.00%) | 85.11 (82.25%) | 📈 view plot 🚷 view threshold | 501.00 (+1.66%) | 501.89 (99.82%) |
| serialize_deserialize_subscribe | 📈 view plot 🚷 view threshold | 23,320.00 (+0.00%) | 23,838.51 (97.82%) | 📈 view plot 🚷 view threshold | 8,129.00 (-0.77%) | 8,314.37 (97.77%) | 📈 view plot 🚷 view threshold | 11,420.00 (-0.99%) | 11,742.46 (97.25%) | 📈 view plot 🚷 view threshold | 42.00 (+9.36%) | 49.91 (84.15%) | 📈 view plot 🚷 view threshold | 334.00 (+0.83%) | 343.13 (97.34%) |
| serialize_submit | 📈 view plot 🚷 view threshold | 99,811.00 (+0.01%) | 100,155.90 (99.66%) | 📈 view plot 🚷 view threshold | 61,325.00 (-0.24%) | 61,737.65 (99.33%) | 📈 view plot 🚷 view threshold | 87,931.00 (-0.29%) | 88,651.53 (99.19%) | 📈 view plot 🚷 view threshold | 52.00 (+9.87%) | 65.49 (79.41%) | 📈 view plot 🚷 view threshold | 332.00 (+2.09%) | 335.84 (98.86%) |
| serialize_subscribe | 📈 view plot 🚷 view threshold | 11,456.00 (+0.46%) | 11,607.50 (98.69%) | 📈 view plot 🚷 view threshold | 4,111.00 (-1.39%) | 4,283.68 (95.97%) | 📈 view plot 🚷 view threshold | 5,686.00 (-1.86%) | 5,996.87 (94.82%) | 📈 view plot 🚷 view threshold | 20.00 (+49.01%) | 23.04 (86.80%) | 📈 view plot 🚷 view threshold | 162.00 (+2.29%) | 163.69 (98.97%) |
|
| Branch | ImproveCoinbase |
| Testbed | sv2 |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result nanoseconds (ns) (Result Δ%) | Upper Boundary nanoseconds (ns) (Limit %) |
|---|---|---|---|
| client_sv2_handle_message_common | 📈 view plot 🚷 view threshold | 44.95 (-0.56%) | 58.90 (76.31%) |
| client_sv2_handle_message_mining | 📈 view plot 🚷 view threshold | 73.56 (-5.77%) | 104.77 (70.21%) |
| client_sv2_mining_message_submit_standard | 📈 view plot 🚷 view threshold | 14.66 (-0.01%) | 14.71 (99.64%) |
| client_sv2_mining_message_submit_standard_serialize | 📈 view plot 🚷 view threshold | 272.98 (+2.17%) | 290.66 (93.92%) |
| client_sv2_mining_message_submit_standard_serialize_deserialize | 📈 view plot 🚷 view threshold | 620.67 (+1.16%) | 650.82 (95.37%) |
| client_sv2_open_channel | 📈 view plot 🚷 view threshold | 162.47 (-1.45%) | 176.23 (92.19%) |
| client_sv2_open_channel_serialize | 📈 view plot 🚷 view threshold | 290.63 (+1.70%) | 311.09 (93.42%) |
| client_sv2_open_channel_serialize_deserialize | 📈 view plot 🚷 view threshold | 378.56 (-1.37%) | 409.47 (92.45%) |
| client_sv2_setup_connection | 📈 view plot 🚷 view threshold | 166.43 (+3.77%) | 172.59 (96.43%) |
| client_sv2_setup_connection_serialize | 📈 view plot 🚷 view threshold | 476.85 (+1.10%) | 551.11 (86.53%) |
| client_sv2_setup_connection_serialize_deserialize | 📈 view plot 🚷 view threshold | 956.98 (-4.02%) | 1,147.03 (83.43%) |
|
| Branch | ImproveCoinbase |
| Testbed | sv2 |
Click to view all benchmark results
| Benchmark | Estimated Cycles | Benchmark Result estimated cycles (Result Δ%) | Upper Boundary estimated cycles (Limit %) | Instructions | Benchmark Result instructions (Result Δ%) | Upper Boundary instructions (Limit %) | L1 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | L2 Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) | RAM Accesses | Benchmark Result accesses (Result Δ%) | Upper Boundary accesses (Limit %) |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| client_sv2_handle_message_common | 📈 view plot 🚷 view threshold | 2,125.00 (+0.77%) | 2,225.21 (95.50%) | 📈 view plot 🚷 view threshold | 473.00 (-0.05%) | 488.96 (96.74%) | 📈 view plot 🚷 view threshold | 735.00 (-0.09%) | 757.93 (96.97%) | 📈 view plot 🚷 view threshold | 5.00 (-0.16%) | 12.16 (41.13%) | 📈 view plot 🚷 view threshold | 39.00 (+1.26%) | 41.52 (93.93%) |
| client_sv2_handle_message_mining | 📈 view plot 🚷 view threshold | 8,216.00 (-0.04%) | 8,379.80 (98.05%) | 📈 view plot 🚷 view threshold | 2,137.00 (-0.01%) | 2,141.28 (99.80%) | 📈 view plot 🚷 view threshold | 3,156.00 (-0.11%) | 3,169.83 (99.56%) | 📈 view plot 🚷 view threshold | 39.00 (+9.86%) | 41.67 (93.58%) | 📈 view plot 🚷 view threshold | 139.00 (-0.36%) | 143.96 (96.55%) |
| client_sv2_mining_message_submit_standard | 📈 view plot 🚷 view threshold | 6,365.00 (+1.06%) | 6,442.86 (98.79%) | 📈 view plot 🚷 view threshold | 1,750.00 (-0.03%) | 1,766.56 (99.06%) | 📈 view plot 🚷 view threshold | 2,545.00 (-0.27%) | 2,575.08 (98.83%) | 📈 view plot 🚷 view threshold | 22.00 (+28.88%) | 24.30 (90.52%) | 📈 view plot 🚷 view threshold | 106.00 (+1.34%) | 108.61 (97.60%) |
| client_sv2_mining_message_submit_standard_serialize | 📈 view plot 🚷 view threshold | 14,720.00 (-0.03%) | 14,936.83 (98.55%) | 📈 view plot 🚷 view threshold | 4,694.00 (-0.01%) | 4,710.56 (99.65%) | 📈 view plot 🚷 view threshold | 6,755.00 (-0.02%) | 6,785.84 (99.55%) | 📈 view plot 🚷 view threshold | 46.00 (+2.87%) | 60.21 (76.40%) | 📈 view plot 🚷 view threshold | 221.00 (-0.11%) | 226.52 (97.56%) |
| client_sv2_mining_message_submit_standard_serialize_deserialize | 📈 view plot 🚷 view threshold | 27,746.00 (+0.60%) | 27,994.57 (99.11%) | 📈 view plot 🚷 view threshold | 10,645.00 (+0.41%) | 10,684.50 (99.63%) | 📈 view plot 🚷 view threshold | 15,506.00 (+0.49%) | 15,570.68 (99.58%) | 📈 view plot 🚷 view threshold | 89.00 (+7.29%) | 97.32 (91.45%) | 📈 view plot 🚷 view threshold | 337.00 (+0.51%) | 342.41 (98.42%) |
| client_sv2_open_channel | 📈 view plot 🚷 view threshold | 4,549.00 (+3.58%) | 4,555.21 (99.86%) | 📈 view plot 🚷 view threshold | 1,461.00 (-0.02%) | 1,476.96 (98.92%) | 📈 view plot 🚷 view threshold | 2,154.00 (-0.30%) | 2,183.68 (98.64%) | 📈 view plot 🚷 view threshold | 10.00 (+22.84%) | 13.85 (72.19%) | 📈 view plot 🚷 view threshold | 67.00 (+7.05%) | 67.16 (99.76%) |
| client_sv2_open_channel_serialize | 📈 view plot 🚷 view threshold | 14,190.00 (+1.22%) | 14,200.51 (99.93%) | 📈 view plot 🚷 view threshold | 5,064.00 (-0.00%) | 5,079.96 (99.69%) | 📈 view plot 🚷 view threshold | 7,320.00 (-0.08%) | 7,351.41 (99.57%) | 📈 view plot 🚷 view threshold | 37.00 (+3.11%) | 46.82 (79.03%) | 📈 view plot 🚷 view threshold | 191.00 (+2.64%) | 191.26 (99.86%) |
| client_sv2_open_channel_serialize_deserialize | 📈 view plot 🚷 view threshold | 22,824.00 (+0.62%) | 22,988.37 (99.28%) | 📈 view plot 🚷 view threshold | 8,040.00 (+0.12%) | 8,053.45 (99.83%) | 📈 view plot 🚷 view threshold | 11,689.00 (+0.05%) | 11,711.00 (99.81%) | 📈 view plot 🚷 view threshold | 85.00 (+13.05%) | 88.11 (96.47%) | 📈 view plot 🚷 view threshold | 306.00 (+0.80%) | 310.85 (98.44%) |
| client_sv2_setup_connection | 📈 view plot 🚷 view threshold | 4,757.00 (+1.48%) | 4,786.88 (99.38%) | 📈 view plot 🚷 view threshold | 1,502.00 (-0.02%) | 1,517.96 (98.95%) | 📈 view plot 🚷 view threshold | 2,272.00 (-0.27%) | 2,299.55 (98.80%) | 📈 view plot 🚷 view threshold | 14.00 (+46.76%) | 15.93 (87.86%) | 📈 view plot 🚷 view threshold | 69.00 (+2.25%) | 70.04 (98.51%) |
| client_sv2_setup_connection_serialize | 📈 view plot 🚷 view threshold | 16,036.00 (-0.69%) | 16,307.86 (98.33%) | 📈 view plot 🚷 view threshold | 5,963.00 (-0.00%) | 5,978.96 (99.73%) | 📈 view plot 🚷 view threshold | 8,661.00 (-0.04%) | 8,691.54 (99.65%) | 📈 view plot 🚷 view threshold | 47.00 (+17.41%) | 54.30 (86.55%) | 📈 view plot 🚷 view threshold | 204.00 (-1.97%) | 212.00 (96.22%) |
| client_sv2_setup_connection_serialize_deserialize | 📈 view plot 🚷 view threshold | 35,634.00 (+0.15%) | 35,890.45 (99.29%) | 📈 view plot 🚷 view threshold | 14,888.00 (+0.16%) | 14,911.09 (99.85%) | 📈 view plot 🚷 view threshold | 21,874.00 (+0.16%) | 21,909.72 (99.84%) | 📈 view plot 🚷 view threshold | 106.00 (+14.06%) | 115.23 (91.99%) | 📈 view plot 🚷 view threshold | 378.00 (-0.36%) | 385.03 (98.17%) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1248 +/- ##
==========================================
+ Coverage 19.29% 19.90% +0.60%
==========================================
Files 164 164
Lines 10852 11025 +173
==========================================
+ Hits 2094 2194 +100
- Misses 8758 8831 +73
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
SInce we're going to use the |
thats the plan |
I'm implementing it, and we need to support at least 2 extranonce if we want to be able to change it while the miner is mining. That cause there will be short time frames where we have job for the new extranonce and job for the old extranonce. I realized that this is true also for the target. We do not support 2 target at the same time, and this is very likley what cause the very rare invalid shares that we observe during tests. I will do another PR to fix it. |
|
I guess it would be better to wait you to push the |
Fi3
force-pushed
the
ImproveCoinbase
branch
6 times, most recently
from
b2dd540 to
0a6f3d7
Compare
Fi3
force-pushed
the
ImproveCoinbase
branch
2 times, most recently
from
0db70f1 to
fd800db
Compare
Coinbase signature is not part of the Sv2 protocol, some pool maybe want to use it other not. The first part of the extranonce could also be reserved for things that are not a pool signature. This pr rename the pool_signature field of the channel factory into additional_coinbase_script_data and change the type from Strgin to Vec<u8>, since can be anything.
The coinbase input script additional data should be sent as part of the extranonce_prefix and not as part of the coinbase_prefix. So that a JDC can see what the pool want as coinbase input script additional data without the need to observ the coinbase prefix in job constructed by the pool.
Update the pool to use an extranonce of 16 bytes rather then 32 so that there is enaugh space to add the additional coinbase input script data.
Right now the channel factory only support one active job at time. That means that if we receive a share for a job right after we sent downstream a new job that share will be invalid. Now the channel factory keep track of the last 3 jobs, so we give time to the dowstream to receive the job and propagate it down before stop accepting shares for older job. This is useful, and the system can be more responsive: as soon as we change the coinbase additional input script data we can send a new job dowsntream without worrying of invalidating miner's shares. When the pool receive a prev hash it immidiatly invalidate all the previous jobs, we still want to refuse shares for stale jobs. The client can easly handle this situation: when a pool refuse a share it should start a timer and if do not receive a new prev hash (or already have) within n seconds it change pool.
This commit fix 2 miner things: When we calculate the coinbase_prefix (what we need to put in the extended job) we need to account also for the coinbase input script additional data that is part of the extranonce. When we create pool channel facotry we pass an extranonce creator and an pool signature. If the signature + extranonce are bigger then 32 bytes we have to return an error. That cause in sv2 the extranonce can not be longer than 32 bytes.
The translator normalize the coinbase and remove the segwit data from the coinbase prefix and suffix. In order to do that it need to know the extranonce len, we used a default value of 322 bytes, but the pool could use also smalle extranonces.
Coinbase signature is not part of the Sv2 protocol, some pool maybe want to use it other not. The first part of the extranonce could also be reserved for things that are not a pool signature. This pr rename the pool_signature field of the channel factory into additional_coinbase_script_data and change the type from Strgin to Vec<u8>, since can be anything.
The coinbase input script additional data should be sent as part of the extranonce_prefix and not as part of the coinbase_prefix. So that a JDC can see what the pool want as coinbase input script additional data without the need to observ the coinbase prefix in job constructed by the pool.
Update the pool to use an extranonce of 16 bytes rather then 32 so that there is enaugh space to add the additional coinbase input script data.
Add a method that the pool can use in order to change coinbase_script_additional_data of an already opened channel and send downstream the new extranonce_prefix
|
Is there an issue/discussion that led to this pull request? |
|
it seems this PR is getting a lot of new commit activity, does it make sense to revert it to draft status until it's ready to go? |
I'm just rebasing it. And fix tests that have been added. No it do not make sense to revert to draft the PR is complete. |
yes we disussed it a lot on discord. Didn't start the discussion on discord, but I propose that we should always use issues and not using other channels like discord or google docs or call. Using them only for things that are not related to changes, or for disccussing for example in chat and call things that alrady have an issue. This is whay I'm trying to do always. Otherwise we end up in siutatuons like that. |
|
@Fi3 Are you planning to put more work here? it seems you need to fix some conflicts and also squash up some commits(probably the ones prefixed with |
|
|
this PR is complete whenever you are willing to review it I can rebase just let me know |
yup, please do. I am in the process of reviewing this. It would be cool if you could combine some of the commits , I think in some cases new code is introduced in an earlier commit and then changed in a later commit, which makes the review process a bit tricky |
|
edit: my bad this PR is not doing anything about JD... apologies for the confusion |
@plebhash |
short Ids haven't yet been removed from spec we only agreed on ideas, but I still need to write the PR to I am a bit behind on this task, I apologize I will try to get a draft PR done by EOW |
|
@plebhash my point is that in this PR we do nothing about shortids that's another one on top o this one. |
|
@Fi3 It is tricky to follow the PR and review it, a lot of big commits you have here. also it seems it combines bug fixes and adding new features. Why not break it to smaller PRs? It will definitely help with speeding up the review process |
|
@jbesraa I can certainly do it but realistically not before 2 weeks from now. If you look at the total diff the actual code changes are not so much, so I would suggested to not review it commit by commit but just the final diff. Something that I could do now is to squash everything together, that could also make sense since every commit in this PR is strictly functional to the goal of this PR. |
|
No rush, we can wait. |
|
have you tried to look at the total diff? is really few changes, take a look there and if you think that it still make sense we can do it, but it will take time. Not sure if we want to wait we have 3 different version of Sv2 right now: braiins SRI and DEMAND all of them incompatible. DEMAND is the most updated one with all the new spec changes, that should be SRI IMO |
you are correct, I apologize for the confusion when I read #1263 (comment) in my mind assumed this PR was also removing the short IDs, but that is not the case |
Better management of the coinbase's input script:
StringtoVec<u8>. Sv2 do not say anything about pool's signature, but we still want to let the user add some arbitrary data and the beginning of the extranonce. We do not assume that this data is a pool signature. We do not assume that this data is valid utf8.