Bugfix: net_processing: Restore "Already requested" error for FetchBlock #28055

luke-jr · 2023-07-09T01:26:25Z

Regression introduced by #27626

Adds new tests

…error, even if we already have the block

…m peer twice

DrahtBot · 2023-07-09T01:26:28Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

No conflicts as of last run.

instagibbs

The result of this PR would be that it would return an error IFF that particular block was requested by that specific peer last. If you request the block from another peer, the behavior would be unchanged. Is that acceptable?

instagibbs · 2023-07-10T19:09:13Z

test/functional/rpc_getblockfrompeer.py

+        slow_peer_id = peers[2]["id"]
+        assert_equal(self.nodes[0].getblockfrompeer(short_tip, slow_peer_id), {})
+        assert_raises_rpc_error(-1, "Already requested from this peer", self.nodes[0].getblockfrompeer, short_tip, slow_peer_id)
+


Suggested change

self.log.info("But every fetch to another peer causes us to forget previous attempts for same block")

self.nodes[0].add_p2p_connection(P2PInterface())

peers = self.nodes[0].getpeerinfo()

assert_equal(len(peers), 4)

slow_peer_id_2 = peers[3]["id"]

assert_equal(self.nodes[0].getblockfrompeer(short_tip, slow_peer_id_2), {})

assert_equal(self.nodes[0].getblockfrompeer(short_tip, slow_peer_id), {})

example test covering the behavior I'm mentioning

mzumsande

The result of this PR would be that it would return an error IFF that particular block was requested by that specific peer last. If you request the block from another peer, the behavior would be unchanged. Is that acceptable?

I think that I would find that acceptable.
I guess one alternative would be to drop the "Already requested from this peer" error that is currently not triggerable, and just forget about the prior request unconditionally on a new request.

Sjors · 2023-07-13T15:54:02Z

I think @mzumsande's suggestion is better. But the current PR, with @instagibbs's tests to document the existing weirdness, is fine too.

Either behaviour is "covered" by the RPC doc warning:

Subsequent calls for the same block may cause the response from the previous peer to be ignored.

Could be improved by saying:

Subsequent calls for the same block may cause the response from the previous ~~peer~~ request to be ignored.

furszy · 2023-08-04T14:57:51Z

src/net_processing.cpp

+    if (IsBlockRequestedFromPeer(hash, peer_id)) return "Already requested from this peer";
+
    // Forget about all prior requests
-    RemoveBlockRequest(block_index.GetBlockHash(), std::nullopt);
+    RemoveBlockRequest(hash, std::nullopt);

    // Mark block as in-flight
-    if (!BlockRequested(peer_id, block_index)) return "Already requested from this peer";
+    Assume(BlockRequested(peer_id, block_index));


In 2b67ea4:

Instead of adding the new function, could just delete the RemoveBlockRequest() line and directly call BlockRequested(). Failing with a Already requested from this peer if the call return false.

BlockRequested() only fails when the block is already in-flight for that peer (it does the same as your new IsBlockRequestedFromPeer() function).

fanquake · 2023-08-17T12:35:59Z

@luke-jr are you going to followup with the questions, test suggestions, code-review and general feedback here?

fanquake · 2023-09-15T09:06:51Z

ping @luke-jr.

fanquake · 2023-10-02T15:11:17Z

Discussed this with some other contributors, and for now, I'm removing this from the 25.1 milestone. I don't currently consider this a blocker for 25.1, and, it cannot be merged or reviewed in any case, until the review feedback is addressed.

maflcko · 2023-10-25T11:29:23Z

Can be marked as draft, while it is waiting on the author?

furszy

I could take this forward if there's no progress here. I actually fixed the issue before this PR was created, in #27837 (27a2592). So, it would just be matter of gathering this PR test commits with my little fix commit in a new PR.

Side shilling note, would be nice to progress on #28170 and #28170 bug fixes.

fanquake · 2023-10-25T13:07:53Z

I could take this forward if there's no progress here.

I don't think it's clear that is something that actually needs fixing, or that we want to change at this point.

furszy · 2023-10-25T13:26:21Z

I could take this forward if there's no progress here.

I don't think it's clear that is something that actually needs fixing, or that we want to change at this point.

well, if the node has requested a block from a certain peer and is waiting for the response. It should not allow the user (or an external software) to re-request the same block multiple times. It is a waste of bandwidth and the other peer could end up banning the node for misbehaving. It's better to notify the misbehavior rather than get silently banned from the network.

DrahtBot · 2024-02-05T14:34:45Z

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

DrahtBot · 2024-06-10T00:51:06Z

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

DrahtBot · 2024-12-31T00:36:19Z

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

fanquake · 2025-01-23T10:35:57Z

Closing, given no followup.

luke-jr added 3 commits July 9, 2023 01:08

Bugfix: net_processing: Restore "Already requested" error for FetchBlock

2b67ea4

QA: rpc_getblockfrompeer: Test that a non-existent peer generates an …

d7e08d4

…error, even if we already have the block

QA: rpc_getblockfrompeer: Test specific error for trying to fetch fro…

017ab85

…m peer twice

This was referenced Jul 9, 2023

net: introduce block tracker to retry to download blocks after failure #27837

Draft

rpc: getblockfrompeer, introduce fetch from "any" functionality #27836

Closed

fanquake requested review from Sjors and instagibbs July 10, 2023 08:19

instagibbs reviewed Jul 10, 2023

View reviewed changes

mzumsande reviewed Jul 11, 2023

View reviewed changes

fanquake added this to the 25.1 milestone Jul 12, 2023

furszy reviewed Aug 4, 2023

View reviewed changes

DrahtBot added the CI failed label Sep 15, 2023

DrahtBot removed the CI failed label Sep 26, 2023

fanquake removed this from the 25.1 milestone Oct 2, 2023

DrahtBot added the CI failed label Oct 25, 2023

furszy reviewed Oct 25, 2023

View reviewed changes

maflcko added Up for grabs and removed Up for grabs labels Feb 5, 2024

fanquake marked this pull request as draft March 6, 2024 17:22

DrahtBot mentioned this pull request Jul 3, 2024

[WIP] p2p: send not_found msgs for unknown, pruned or unwilling to share blocks #30385

Closed

fanquake closed this Jan 23, 2025

+        self.log.info("But every fetch to another peer causes us to forget previous attempts for same block")
+        self.nodes[0].add_p2p_connection(P2PInterface())
+        peers = self.nodes[0].getpeerinfo()
+        assert_equal(len(peers), 4)
+        slow_peer_id_2 = peers[3]["id"]
+        assert_equal(self.nodes[0].getblockfrompeer(short_tip, slow_peer_id_2), {})
+        assert_equal(self.nodes[0].getblockfrompeer(short_tip, slow_peer_id), {})

Bugfix: net_processing: Restore "Already requested" error for FetchBlock #28055

Bugfix: net_processing: Restore "Already requested" error for FetchBlock #28055

Uh oh!

Conversation

luke-jr commented Jul 9, 2023

Uh oh!

DrahtBot commented Jul 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

instagibbs Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

furszy Aug 4, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake commented Aug 17, 2023

Uh oh!

fanquake commented Sep 15, 2023

Uh oh!

fanquake commented Oct 2, 2023

Uh oh!

maflcko commented Oct 25, 2023

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Oct 25, 2023

Uh oh!

furszy commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 5, 2024

Uh oh!

DrahtBot commented Jun 10, 2024

Uh oh!

DrahtBot commented Dec 31, 2024

Uh oh!

fanquake commented Jan 23, 2025

Uh oh!

Uh oh!

DrahtBot commented Jul 9, 2023 •

edited

Loading

Sjors commented Jul 13, 2023 •

edited

Loading

furszy commented Oct 25, 2023 •

edited

Loading