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

netsync: Rework inventory announcement handling. #2548

Merged
merged 2 commits into from
Jan 15, 2021

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Jan 11, 2021

This is rebased on #2547.

This modifies the way inventory announcements are handled to improve efficiency and readability.

Specifically, it switches to a more explicit approach that handles the specific recognized types (blocks and transactions) independently and now determines if they are needed with unique logic for each type as opposed to the more generic inventory-only based approach.

Next, and probably the most important change overall, is that recently confirmed transactions are now tracked by the server and the logic which determines if a transaction is needed now makes use of those tracked transactions as opposed to the previous rather expensive utxo-based query approach.

The reasoning for this change is that the only time honest nodes notify others about transactions are when they view those transactions as unconfirmed and they consider themselves to be current. In practice this means the only duplicate announcements are for unconfirmed and recently confirmed transactions. In the case of malicious nodes, the transactions are rejected (either the transactions are actually invalid or they are old transactions that were valid in the past but will now fail due to trying to spend outputs they already spent).

So, given the above discussion, there are 3 cases to handle:

  1. Duplicate announcements for unconfirmed transactions
  2. Duplicate announcements for txns that have already been rejected
  3. Duplicate announcements for recently-confirmed transactions

For the first case, the duplicate request is filtered by the mempool since it already knows the unconfirmed transaction. For the second case, rejected transactions are tracked separately and filtered. Thus, only the third case of recently confirmed transactions remains.

@davecgh davecgh added this to the 1.7.0 milestone Jan 11, 2021
@davecgh davecgh force-pushed the netsync_rework_inv_announcement branch from 1444559 to 6488fb9 Compare January 11, 2021 00:50
Copy link
Member

@rstaudt2 rstaudt2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, definitely simplifies the case for the recently confirmed transaction handling.

internal/netsync/manager.go Outdated Show resolved Hide resolved
internal/netsync/manager.go Outdated Show resolved Hide resolved
@davecgh davecgh force-pushed the netsync_rework_inv_announcement branch from 6488fb9 to 33c39f2 Compare January 11, 2021 17:34
This removes the error return from the LatestBlockLocator function since
it can never fail and ends up causing unneeded error checking.
This modifies the way inventory announcements are handled to improve
efficiency and readability.

Specifically, it switches to a more explicit approach that handles the
specific recognized types (blocks and transactions) independently and
now determines if they are needed with unique logic for each type as
opposed to the more generic inventory-only based approach.

Next, and probably the most important change overall, is that recently
confirmed transactions are now tracked by the server and the logic which
determines if a transaction is needed now makes use of those tracked
transactions as opposed to the previous rather expensive utxo-based
query approach.

The reasoning for this change is that the only time honest nodes notify
others about transactions are when they view those transactions as
unconfirmed and they consider themselves to be current.  In practice
this means the only duplicate announcements are for unconfirmed and
recently confirmed transactions.  In the case of malicious nodes, the
transactions are rejected (either the transactions are actually invalid
or they are old transactions that were valid in the past but will now
fail due to trying to spend outputs they already spent).

So, given the above discussion, there are 3 cases to handle:

1) Duplicate announcements for unconfirmed transactions
2) Duplicate announcements for txns that have already been rejected
3) Duplicate announcements for recently-confirmed transactions

For the first case, the duplicate request is filtered by the mempool
since it already knows the unconfirmed transaction.  For the second
case, rejected transactions are tracked separately and filtered.  Thus,
only the third case of recently confirmed transactions remains.

Finally, the last block is now tracked by the hash instead of an index
into the index which allows the relevant updates to take place after the
main loop versus inside of it conditionally.
@davecgh davecgh force-pushed the netsync_rework_inv_announcement branch from 33c39f2 to c93e1a0 Compare January 15, 2021 04:25
@davecgh davecgh merged commit c93e1a0 into decred:master Jan 15, 2021
@davecgh davecgh deleted the netsync_rework_inv_announcement branch January 15, 2021 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants