Skip to content
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

Add is_originator flag to publish messages #4654

Merged
merged 4 commits into from
Jul 15, 2024

Conversation

pwojcikdev
Copy link
Contributor

This adds a new flag to publish messages that indicates whether a block is coming from a node that performed initial flooding. This helps avoid a situation when under a high load, a legitimate block is dropped because blockprocessor queue for a peer is full of rebroadcasted blocks. However, the potential for exploiting this is limited, as both rebroadcasted and originator queues have the same max size and priority, so effectively this gives each peer two queues to spam the network - which is not much, but should be plenty for legitimate traffic.

Format of extended publish message:

/*
 * Binary Format:
 * [message_header] Common message header
 * [variable] Block (serialized according to the block type specified in the header)
 *
 * Header extensions:
 * - [0x0f00] Block type: Identifies the specific type of the block.
 * - [0x0004] Originator flag
 */

@pwojcikdev pwojcikdev requested a review from clemahieu June 23, 2024 20:35
@pwojcikdev
Copy link
Contributor Author

Posting @gr0vity-dev test results here for reference ("heuristic" run refers to this PR):

I did a comparison for these, each time on a 10 node network with default votes/msg :
LMDB - large Q - heuristic
LMDB - large Q - develop
RocksDB - large Q - heuristic
RocksDB - large Q - develop
RocksDB performs better than LMDB.
is_originator seems to have a small positiv impact on the duration until a block is seen. It's most noticable in the LMDB run, where sometimes it takes 10s for a node to see the block on develop, but mostly below 2s with the new flag.

Below the column "seen" is of the most interest as it indicates how long it took to propagate a block across the network:
image
image

gr0vity-dev pushed a commit to gr0vity-dev/nano-node that referenced this pull request Jun 26, 2024
gr0vity-dev pushed a commit to gr0vity-dev/nano-node that referenced this pull request Jun 26, 2024
gr0vity-dev pushed a commit to gr0vity-dev/nano-node that referenced this pull request Jun 26, 2024
@clemahieu
Copy link
Contributor

One thing that stood out is that with confirm_ack we have is_rebroadcasted but with publish we have is_originator. For the same reason we used is_rebroadcasted shouldn't we use is_rebroadcasted on publish as well? Basically, the flag is an opt-in by the sender saying the block is of lower priority. We can't prove a sender is the originator but they can opt-in to be lower priority.

@pwojcikdev
Copy link
Contributor Author

pwojcikdev commented Jul 2, 2024

Either of these flags could work. With confirm_acks there was a compatibility constraint where votes needed to be treated as non-rebroadcasted by default, that's why the is_rebroadcasted flag is opt-in. Here there is no such requirement, but treating blocks as lower priority (rebroadcasted) by default is less error prone. It only requires ensuring that the is_originator flag is set in a single place (local_block_broadcaster).

@qwahzi qwahzi added this to the V27 milestone Jul 2, 2024
# Conflicts:
#	nano/node/blockprocessor.hpp
gr0vity-dev pushed a commit to gr0vity-dev/nano-node that referenced this pull request Jul 7, 2024
@pwojcikdev pwojcikdev merged commit 1cd5564 into nanocurrency:develop Jul 15, 2024
22 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged / V27.0
Development

Successfully merging this pull request may close these issues.

3 participants