-
Notifications
You must be signed in to change notification settings - Fork 130
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
Improve coinbase #1248
base: main
Are you sure you want to change the base?
Improve coinbase #1248
Conversation
Bencher Report
🚨 1 Alert
Click to view all benchmark results
|
Bencher Report
Click to view all benchmark results
|
Bencher Report
🚨 3 Alerts
Click to view all benchmark results
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1248 +/- ##
==========================================
+ Coverage 19.30% 19.90% +0.60%
==========================================
Files 164 164
Lines 10849 11021 +172
==========================================
+ Hits 2094 2194 +100
- Misses 8755 8827 +72
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 |
08cf9a2
to
6734198
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.
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
4102974
to
b2dd540
Compare
Better management of the coinbase's input script:
String
toVec<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.