-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
src: dynamic block sync size #9494
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
base: master
Are you sure you want to change the base?
src: dynamic block sync size #9494
Conversation
5b05971
to
ce38fde
Compare
Same content. Only removed white-space changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! It was helpful for me to start reviewing out from here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on stressnet and mainnet
ce38fde
to
2ce35d9
Compare
2ce35d9
to
03d0dce
Compare
All comments have been addressed. Once discussion finished, I will update |
03d0dce
to
2195b6a
Compare
Updated to most latest |
2195b6a
to
5459934
Compare
Rebased to updated master. Now CI should run without any issue. |
MINFO("Last " << number_of_blocks | ||
<< " blocks median size is " << median_weight | ||
<< " bytes and the max average blocksize in the queue is " << average_blocksize_of_biggest_batch << " bytes"); | ||
uint64_t projected_blocksize = (average_blocksize_of_biggest_batch > median_weight) ? average_blocksize_of_biggest_batch : median_weight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uint64_t projected_blocksize = (average_blocksize_of_biggest_batch > median_weight) ? average_blocksize_of_biggest_batch : median_weight; | |
uint64_t projected_blocksize = (average_blocksize_of_biggest_batch > median_weight) ? average_blocksize_of_biggest_batch : median_weight; | |
uint64_t blocks_huge_threshold = (batch_max_weight / 2); |
else if (projected_blocksize >= batch_max_weight) { | ||
res = 1; | ||
MINFO("blocks are projected to surpass " << batch_max_weight << " bytes, syncing just a single block in next batch"); | ||
} | ||
else if (projected_blocksize > BLOCKS_HUGE_THRESHOLD_SIZE) { | ||
res = 1; | ||
MINFO("blocks are huge, sync just a single block in next batch"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if (projected_blocksize >= batch_max_weight) { | |
res = 1; | |
MINFO("blocks are projected to surpass " << batch_max_weight << " bytes, syncing just a single block in next batch"); | |
} | |
else if (projected_blocksize > BLOCKS_HUGE_THRESHOLD_SIZE) { | |
res = 1; | |
MINFO("blocks are huge, sync just a single block in next batch"); | |
else if (projected_blocksize >= blocks_huge_threshold) { | |
res = 1; | |
MINFO("blocks are projected to surpass 50% of " << batch_max_weight << " bytes, syncing just a single block in next batch"); | |
} |
condense
src/cryptonote_config.h
Outdated
#define BLOCKS_MEDIAN_WINDOW 100 //by default, compute median weights of last 100 blocks | ||
#define BATCH_MAX_WEIGHT 20 //by default, maximum size of batch in [mB] | ||
#define BATCH_MAX_ALLOWED_WEIGHT 50 //maximum allowed size of batch in [mB] | ||
#define BLOCKS_HUGE_THRESHOLD_SIZE ((BATCH_MAX_WEIGHT * 1000000) / 2) //blocks that we consider huge [B] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define BLOCKS_HUGE_THRESHOLD_SIZE ((BATCH_MAX_WEIGHT * 1000000) / 2) //blocks that we consider huge [B] |
swapped here
#9494 (comment)
src/cryptonote_config.h
Outdated
@@ -98,6 +98,10 @@ | |||
#define BLOCKS_SYNCHRONIZING_DEFAULT_COUNT_PRE_V4 100 //by default, blocks count in blocks downloading | |||
#define BLOCKS_SYNCHRONIZING_DEFAULT_COUNT 20 //by default, blocks count in blocks downloading | |||
#define BLOCKS_SYNCHRONIZING_MAX_COUNT 2048 //must be a power of 2, greater than 128, equal to SEEDHASH_EPOCH_BLOCKS | |||
#define BLOCKS_MEDIAN_WINDOW 100 //by default, compute median weights of last 100 blocks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#define BLOCKS_MEDIAN_WINDOW 100 //by default, compute median weights of last 100 blocks | |
#define BLOCKS_MEDIAN_WINDOW CRYPTONOTE_REWARD_BLOCKS_WINDOW //by default, compute median weights of last 100 blocks (CRYPTONOTE_REWARD_BLOCKS_WINDOW) |
@@ -112,6 +113,15 @@ namespace cryptonote | |||
void log_connections(); | |||
std::list<connection_info> get_connections(); | |||
const block_queue &get_block_queue() const { return m_block_queue; } | |||
const std::uint64_t max_average_of_blocksize_in_queue() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const std::uint64_t max_average_of_blocksize_in_queue() { | |
std::uint64_t max_average_of_blocksize_in_queue() { |
Warning in build log
03bcd6f
to
fd06f00
Compare
a4b2894
to
dc80b29
Compare
a71734d
to
ad988af
Compare
Squashed to single commit. No change in content. |
e405405
to
0a498c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ready. I've tested numerous times
@vtnerd can you take another look
@@ -192,6 +202,7 @@ namespace cryptonote | |||
uint64_t m_sync_download_chain_size, m_sync_download_objects_size; | |||
size_t m_block_download_max_size; | |||
bool m_sync_pruned_blocks; | |||
std::atomic<size_t> m_bss; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only see 1 write and no reads of this variable, can it be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in this PR.
But in the dynamic span PR we need this variable. Long story short, it is fine to delete it here.
@@ -112,6 +113,15 @@ namespace cryptonote | |||
void log_connections(); | |||
std::list<connection_info> get_connections(); | |||
const block_queue &get_block_queue() const { return m_block_queue; } | |||
std::uint64_t max_average_of_blocksize_in_queue() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nitpick, but this bracing style isn't matching to this document. Same thing in other files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course. Will be fixed in updated push.
MINFO("Max block size seen within the last " << number_of_blocks | ||
<< " blocks is " << max_weight | ||
<< " bytes and the max average blocksize in the queue is " << max_average_of_blocksize_in_queue << " bytes"); | ||
uint64_t projected_blocksize = (max_average_of_blocksize_in_queue > max_weight) ? max_average_of_blocksize_in_queue : max_weight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a std::max(max_average_of_blocksize_in_queue, max_weight)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Will be fixed in updated PR.
<< " bytes and the max average blocksize in the queue is " << max_average_of_blocksize_in_queue << " bytes"); | ||
uint64_t projected_blocksize = (max_average_of_blocksize_in_queue > max_weight) ? max_average_of_blocksize_in_queue : max_weight; | ||
uint64_t blocks_huge_threshold = (batch_max_weight / 2); | ||
if ((projected_blocksize * BLOCKS_MAX_WINDOW) < batch_max_weight) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this overflow? Does it make sense to flip the logic to:
if (projected_block_size < batch_max_weight / BLOCKS_MAX_WINDOW)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not.
projected_block_size
is uint64_t.
BLOCKS_MAX_WINDOW
is 100.
batch_max_weight
is uint64_t.
You do the math. Basically impossible with current ( and foreseeable ) network limitations to get close to overflow.
MINFO("blocks are tiny, " << projected_blocksize << " bytes, sync " << res << " blocks in next batch"); | ||
} | ||
else if (projected_blocksize >= blocks_huge_threshold) { | ||
res = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the block sizes exceed 5MB, then it drops down to one block? The max levin size is 100 MB? These values seem to too low.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do have a serialization limit, too. So, because of the serialization limit, we have to limit it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- By default the estimations target 10mb batches
- estimates are conservative, but can still be exceeded by ~100% under spike volume
- current serialization limits break syncing if you exceed ~25mb in a batch
- until serialization limits are extended / removed, 10mb is the largest we can safely default to. Any larger has potential to hit serialization limits
- if a single block is expected to exceed 1/2 of the batch size target, then syncing more than 1 block would exceed the target, hence the "5mb"
- the max this can be set to, is 50mb (1/2 of the levin limit to add safe-zone for underestimation due to spikes in volume). 50mb cant be used today though, because of serialization
Also to note
- downloading of 10mb batches results in recalculating the estimation more often, and more reliable downloads (since monero likes to ban peers who dont complete the upload fast enough)
0a498c6
to
3096729
Compare
MWARNING("When --block-sync-size defined, the --batch-max-weight is not going to have any effect."); | ||
|
||
batch_max_weight = command_line::get_arg(vm, arg_batch_max_weight); | ||
if (batch_max_weight > BATCH_MAX_ALLOWED_WEIGHT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bracing in this section
res = BLOCKS_SYNCHRONIZING_DEFAULT_COUNT; | ||
else | ||
res = BLOCKS_SYNCHRONIZING_DEFAULT_COUNT_PRE_V4; | ||
else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bracing in this section
@@ -192,6 +203,7 @@ namespace cryptonote | |||
uint64_t m_sync_download_chain_size, m_sync_download_objects_size; | |||
size_t m_block_download_max_size; | |||
bool m_sync_pruned_blocks; | |||
std::atomic<size_t> m_bss; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove? #9494 (comment)
f54a254
to
3e36c30
Compare
Co-authored-by: nahuhh
3e36c30
to
2a1a489
Compare
No description provided.