make Pool's CLIENT_SEARCH_SPACE_BYTES more permissive#1977
Conversation
|
|
||
| const POOL_ALLOCATION_BYTES: usize = 4; | ||
| const CLIENT_SEARCH_SPACE_BYTES: usize = 8; | ||
| const CLIENT_SEARCH_SPACE_BYTES: usize = 16; |
There was a problem hiding this comment.
there's no fundamental reason preventing us from doing that
however, that's highly suggestive of the misconception that full extranonces must be fixed at 32 bytes, which in turn makes us highly susceptible to blindspots
in fact, I foresee some challenges around this topic when we start making adaptations coming from stratum-mining/sv2-spec#162
so I'd like to avoid that
There was a problem hiding this comment.
Ah cool!!! let me check the spec stuff. Consider it non-blocking.
e275b15 to
e9b97a8
Compare
| jdc_pool_sniffer | ||
| .wait_for_message_type( | ||
| MessageDirection::ToUpstream, | ||
| MESSAGE_TYPE_OPEN_EXTENDED_MINING_CHANNEL, | ||
| ) | ||
| .await; | ||
| jdc_pool_sniffer | ||
| .wait_for_message_type( | ||
| MessageDirection::ToDownstream, | ||
| MESSAGE_TYPE_OPEN_EXTENDED_MINING_CHANNEL_SUCCESS, | ||
| ) | ||
| .await; | ||
| jdc_pool_sniffer | ||
| .wait_for_message_type( | ||
| MessageDirection::ToUpstream, | ||
| MESSAGE_TYPE_SUBMIT_SHARES_EXTENDED, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
This test doesn’t actually verify the behavior in non-aggregated mode. Ideally, we should check whether multiple mining instance connections result in multiple OpenExtendedMiningChannel messages.
There was a problem hiding this comment.
how can we achieve that?
unfortunately asserting for two identical messages is not trivial...
if we check the inner workings of wait_for_message_type, we'll see that it's redundant to call it 2x for the same message type
I tried doing wait_for_message_type_and_clean_queue but the test started timing out
I see how the test is asserting less than ideal
but overall, my goal with adding this test was just to make sure the original edge case that motivated this PR would be caught on CI
if we keep the test and revert the change on pool, it should crash
There was a problem hiding this comment.
wait, just realized I was doing some dumb stuff
e9b97a8 to
bead6c5
Compare
|
migrated to stratum-mining/sv2-apps#47 |
close stratum-mining/sv2-apps#44
since tProxy will likely operate with
downstream_extranonce2_sizeat most 8, allowing total of 16 bytes for Pool's clients should be sufficient for most cases