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

luke-jr commented at 1:26 AM on July 9, 2023: member

Regression introduced by #27626

Adds new tests

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

QA: rpc_getblockfrompeer: Test that a non-existent peer generates an error, even if we already have the block d7e08d408c

QA: rpc_getblockfrompeer: Test specific error for trying to fetch from peer twice 017ab85cec

DrahtBot commented at 1:26 AM on July 9, 2023: contributor

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.

fanquake requested review from Sjors on Jul 10, 2023

fanquake requested review from instagibbs on Jul 10, 2023

in test/functional/rpc_getblockfrompeer.py:94 in 017ab85cec

  89 | +        peers = self.nodes[0].getpeerinfo()
  90 | +        assert_equal(len(peers), 3)
  91 | +        slow_peer_id = peers[2]["id"]
  92 | +        assert_equal(self.nodes[0].getblockfrompeer(short_tip, slow_peer_id), {})
  93 | +        assert_raises_rpc_error(-1, "Already requested from this peer", self.nodes[0].getblockfrompeer, short_tip, slow_peer_id)
  94 | +

instagibbs commented at 7:09 PM on July 10, 2023:

        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), {})

instagibbs commented at 7:11 PM on July 10, 2023:

example test covering the behavior I'm mentioning

instagibbs commented at 7:11 PM on July 10, 2023: member

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?

mzumsande commented at 7:43 PM on July 11, 2023: contributor

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.

fanquake added this to the milestone 25.1 on Jul 12, 2023

Sjors commented at 3:54 PM on July 13, 2023: member

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.

in src/net_processing.cpp:1807 in 2b67ea465c outdated

1804 | -    RemoveBlockRequest(block_index.GetBlockHash(), std::nullopt);
1805 | +    RemoveBlockRequest(hash, std::nullopt);
1806 |  
1807 |      // Mark block as in-flight
1808 | -    if (!BlockRequested(peer_id, block_index)) return "Already requested from this peer";
1809 | +    Assume(BlockRequested(peer_id, block_index));

furszy commented at 2:57 PM on August 4, 2023:

In 2b67ea46:

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 commented at 12:35 PM on August 17, 2023: member

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

fanquake commented at 9:06 AM on September 15, 2023: member

ping @luke-jr.

DrahtBot added the label CI failed on Sep 15, 2023

DrahtBot removed the label CI failed on Sep 26, 2023

fanquake removed this from the milestone 25.1 on Oct 2, 2023

fanquake commented at 3:11 PM on October 2, 2023: member

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.

DrahtBot added the label CI failed on Oct 25, 2023

maflcko commented at 11:29 AM on October 25, 2023: member

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

furszy commented at 12:28 PM on October 25, 2023: member

I could take this forward if there's no progress here. I actually fixed the issue before this PR was created, in #27837 (27a25929c78). 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 commented at 1:07 PM on October 25, 2023: member

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 commented at 1:26 PM on October 25, 2023: member

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 commented at 2:34 PM on February 5, 2024: contributor

🤔 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.

maflcko added the label Up for grabs on Feb 5, 2024

maflcko removed the label Up for grabs on Feb 5, 2024

fanquake marked this as a draft on Mar 6, 2024

DrahtBot commented at 12:51 AM on June 10, 2024: contributor

🤔 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 commented at 12:36 AM on December 31, 2024: contributor

🤔 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 commented at 10:35 AM on January 23, 2025: member

Closing, given no followup.

fanquake closed this on Jan 23, 2025

bitcoin locked this on Jan 23, 2026