Braiins/cross testing #1396
Braiins/cross testing #1396
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1396 +/- ##
==========================================
+ Coverage 19.08% 19.20% +0.11%
==========================================
Files 166 166
Lines 11066 11107 +41
==========================================
+ Hits 2112 2133 +21
- Misses 8954 8974 +20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
|
cACK |
| @@ -2,7 +2,7 @@ | |||
| use alloc::vec::Vec; | |||
| #[cfg(not(feature = "with_serde"))] | |||
| use binary_sv2::binary_codec_sv2; | |||
| use binary_sv2::{Deserialize, Seq0255, Serialize, Sv2Option, B032, B064K, U256}; | |||
| use binary_sv2::{Deserialize, Seq0255, Serialize, Sv2Option, B064K, U256}; | |||
There was a problem hiding this comment.
If B032 doesn't exist in the spec, maybe binary_sv2 shouldn't define it?
More rigorously, should it wiped from the codebase?
There was a problem hiding this comment.
B32 doesn't exist in the spec.
B0_32 does exist in the spec.
| Data Type | Byte Length | Description |
|---|---|---|
| B0_32 | 1 + LENGTH | Byte array with 8-bit length prefix L. Unsigned integer, followed by a sequence of L bytes. Allowed range of length is 0 to 32. |
https://stratumprotocol.org/specification/03-Protocol-Overview/#31-data-types-mapping
There was a problem hiding this comment.
Oh I see, it's still used for extranonce and extranonce_prefix.
Out of scope for this PR, but is it really worth having a separate B0_32 datatype instead of just using B0_255 and then having the spec limit the extranonce size to 31?
It would be less confusing, because extranonce_size is U16 which allows for much bigger values. (U8 would have made more sense, but not worth changing now).
Adding an explanation as to why that limit was picked would be good too.
There was a problem hiding this comment.
I think the reason behind the limit of 32 bytes was set because it's enough to use it in a custom way from the pool (e.g. setting the pool tag in the coinbase tx), while letting the rest of the space to the miner to use it as real extranonce space.
plebhash
force-pushed
the
braiins/cross-testing
branch
from
63d214b to
0702b14
Compare
plebhash
force-pushed
the
braiins/cross-testing
branch
2 times, most recently
from
25ed4a8 to
5502fe7
Compare
plebhash
force-pushed
the
braiins/cross-testing
branch
from
5502fe7 to
85998cc
Compare
- Fix unit test new_mining_job_serialization - Add testcase for NewMiningJob serialization
plebhash
force-pushed
the
braiins/cross-testing
branch
from
85998cc to
f54512a
Compare
Fix for #1395
There has been some incompatibility between braiins miner and SRI pool. I was looking into it and found the root cause of the issue. It was in Merkle Root.
Please note that the unit tests are probably not in the correct place. I didn't have time to try to find the best place for them.