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.

Sign up for GitHub

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

Jump to bottom

Implement RemoveAskFor to indicate that we're not interested in an item anymore #2384

Merged
merged 3 commits into from
Oct 26, 2018
Merged

Implement RemoveAskFor to indicate that we're not interested in an item anymore #2384

merged 3 commits into from
Oct 26, 2018

Conversation

codablock
Copy link

When an INV item is received from the first node, the item is requested
immediately. If the same item is received from another node, an entry is
added to mapAskFor which marks the item for re-requesting in case the first
node did not respond. When the item is received from the first node,
the item was previously never removed from mapAskFor. Only the later getdata
loop in SendMessages would then gradually remove items from the map. This
is quite delayed however as the entries in mapAskFor have a timeout value.

RemoveAskFor allows to remove all entries from mapAskFor and setAskFor
when we are not interested in the item anymore (e.g. because we received
it already).

@codablock
Copy link
Author

Forgot to describe the reason I implemented this in the first place:
Very often when doing some tests with debug=1, I noticed hundreds of message like:

2014-12-04 17:20:30 SendMessages -- GETDATA -- already have inv = mnw f7f5f6b111c845b8598e10989d87ac36597d1c11bca317ba0bbd768ba0ce722b peer=9
2014-12-04 17:20:30 SendMessages -- GETDATA -- already have inv = mnw aa64a0d21f9e7e2d24bb8d867cf3ec3c2d48fe02989c3a32b5254503e43ad561 peer=9
2014-12-04 17:20:30 SendMessages -- GETDATA -- already have inv = mnw ee8aa773c15727792a90be2c67f72701c8980e931f863234d18bd818f08fcf60 peer=10
2014-12-04 17:20:30 SendMessages -- GETDATA -- already have inv = mnw a8feea15102d83e892b4aee05ab2abd44a212796f2b7d02cc796b01a04a95c3d peer=10
2014-12-04 17:20:30 SendMessages -- GETDATA -- already have inv = mnw 4a5e52b8d163cde39146e4c25cd8de2db1b3ab0dcbd1c61d4a7ddc62c2fc9510 peer=10
2014-12-04 17:20:30 SendMessages -- GETDATA -- already have inv = mnw c574704f33891f3f7e23c21118b6a43ddc8d9cd3e2bdc7d4f80301d51baa677e peer=10
2014-12-04 17:20:30 SendMessages -- GETDATA -- already have inv = mnw 5a36b789b4ad1bf452829485b1be10f172ae38541a6a8b9a82f822130fbe982d peer=10
2014-12-04 17:20:30 SendMessages -- GETDATA -- already have inv = mnw 7433f4519cdfeb6c51963aaa238e2d928ce6f046e0b44eb716f05bebaf2807d4 peer=10
2014-12-04 17:20:30 SendMessages -- GETDATA -- already have inv = mnw 38ec50d06fff7577cd610db8319ab10cd7229dc463a3e639a398b2fbae542664 peer=10
2014-12-04 17:20:30 SendMessages -- GETDATA -- already have inv = mnw 267ada324fecdac9abdf253fcb98196eb2eccc14dcabdb53454c2af294d4656e peer=10

I always found these very confusing because it looks like the node is trying to re-request some inventory items or that it received duplicated INV items long after the message was received. Mostly noticed this my LLMQ branch where a lot of message are exchanged in intra-quorum communication, but now also observed this in the auto IX test cases, so I decided to backport this commit from my local LLMQ branch.

@codablock
Implement RemoveAskFor to indicate that we're not interested in an it…
4987128 
…em anymore

When an INV item is received from the first node, the item is requested
immediately. If the same item is received from another node, an entry is
added to mapAskFor which marks the item for re-requesting in case the first
node did not respond. When the item is received from the first node,
the item was previously never removed from mapAskFor. Only the later getdata
loop in SendMessages would then gradually remove items from the map. This
is quite delayed however as the entries in mapAskFor have a timeout value.

RemoveAskFor allows to remove all entries from mapAskFor and setAskFor
when we are not interested in the item anymore (e.g. because we received
it already).
@codablock
Call RemoveAskFor whenever we receive a message
b35ee5c
@UdjinM6 UdjinM6 added this to the 13.0 milestone Oct 26, 2018
@UdjinM6
Copy link

UdjinM6 commented Oct 26, 2018

Looks good in general but why constructing and passing a CInv object each time even though only a hash of it is used later in code?

@codablock
Copy link
Author

@UdjinM6 True, didn't make sense. Using uint256 now.

UdjinM6
UdjinM6 requested changes Oct 26, 2018
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

The last commit feels incomplete... see inline comments.

src/instantx.cpp Outdated
pfrom->setAskFor.erase(nVoteHash);
{
LOCK(cs_main);
connman.RemoveAskFor(CInv(MSG_TXLOCK_VOTE, nVoteHash));
Copy link

Choose a reason for hiding this comment

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

still uses CInv?

Copy link
Author

Choose a reason for hiding this comment

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

Strange. Looks like I did the global search too early after branch switching (CLion indexes out of date maybe). Fixing in a squashed commit.

src/masternode-payments.cpp Outdated
pfrom->setAskFor.erase(nHash);
{
LOCK(cs_main);
connman.RemoveAskFor(CInv(MSG_MASTERNODE_PAYMENT_VOTE, nHash));
Copy link

Choose a reason for hiding this comment

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

same

src/masternodeman.cpp Outdated
pfrom->setAskFor.erase(mnb.GetHash());
{
LOCK(cs_main);
connman.RemoveAskFor(CInv(MSG_MASTERNODE_ANNOUNCE, mnb.GetHash()));
Copy link

Choose a reason for hiding this comment

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

same + 2 more below

UdjinM6
UdjinM6 approved these changes Oct 26, 2018
View reviewed changes
Copy link

@UdjinM6 UdjinM6 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 now 👍

utACK

@UdjinM6 UdjinM6 merged commit b5142ee into dashpay:develop Oct 26, 2018
@codablock codablock deleted the pr_removeaskfor branch December 27, 2018 15:24
CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Jun 29, 2020
@codablock @CryptoCentric
Implement RemoveAskFor to indicate that we're not interested in an it…
5425bc0 
…em anymore (dashpay#2384)

* Implement RemoveAskFor to indicate that we're not interested in an item anymore

When an INV item is received from the first node, the item is requested
immediately. If the same item is received from another node, an entry is
added to mapAskFor which marks the item for re-requesting in case the first
node did not respond. When the item is received from the first node,
the item was previously never removed from mapAskFor. Only the later getdata
loop in SendMessages would then gradually remove items from the map. This
is quite delayed however as the entries in mapAskFor have a timeout value.

RemoveAskFor allows to remove all entries from mapAskFor and setAskFor
when we are not interested in the item anymore (e.g. because we received
it already).

* Call RemoveAskFor whenever we receive a message

* Only pass hash instead of CInv object to RemoveAskFor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
13.0
Development

Successfully merging this pull request may close these issues.
2 participants
@codablock @UdjinM6