[{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/492832470","pull_request_review_id":493590059,"id":492832470,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MjgzMjQ3MA==","diff_hunk":"@@ -0,0 +1,226 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"2cac00639e5eb22eb6b076bcc001196d958a4f2e","user":{"login":"maflcko","id":6399679,"node_id":"MDQ6VXNlcjYzOTk2Nzk=","avatar_url":"https://avatars.githubusercontent.com/u/6399679?v=4","gravatar_id":"","url":"https://api.github.com/users/maflcko","html_url":"https://github.com/maflcko","followers_url":"https://api.github.com/users/maflcko/followers","following_url":"https://api.github.com/users/maflcko/following{/other_user}","gists_url":"https://api.github.com/users/maflcko/gists{/gist_id}","starred_url":"https://api.github.com/users/maflcko/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/maflcko/subscriptions","organizations_url":"https://api.github.com/users/maflcko/orgs","repos_url":"https://api.github.com/users/maflcko/repos","events_url":"https://api.github.com/users/maflcko/events{/privacy}","received_events_url":"https://api.github.com/users/maflcko/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"```suggestion\r\n * - Whether it's from a \"preferred\" peer or not (outbound and noban peers are preferred).\r\n```\r\n\r\nstyle-nit: Would be good to mention the exact permission flag, because legacy whitelisted is discouraged, has been deprecated, and might be removed some time in the future.\r\n\r\n(Same below)","created_at":"2020-09-22T15:30:39Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r492832470","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/492832470"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r492832470"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/492832470/reactions","total_count":1,"+1":1,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":28,"side":"RIGHT","author_association":"MEMBER","original_position":28,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493158786","pull_request_review_id":493984643,"id":493158786,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE1ODc4Ng==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"`exptime` is slightly ambiguous with \"expected time\" to me, maybe replace with \"timeout\" or \"expiry_time\" or just \"expiry\" ?","created_at":"2020-09-23T02:23:39Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493158786","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493158786"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493158786"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493158786/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":30,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":30,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493160848","pull_request_review_id":493984643,"id":493160848,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE2MDg0OA==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"\"(see further for details)\" is true of all these points, I think.","created_at":"2020-09-23T02:30:44Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493160848","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493160848"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493160848"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493160848/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":29,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":29,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493161600","pull_request_review_id":493984643,"id":493161600,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE2MTYwMA==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"This was confusing -- I read it as \"the first of many markers\" rather than \"a marker indicating it was first\" and wondered what these \"markers\" were. Adding quotes around \"first\" consistently might be better?","created_at":"2020-09-23T02:33:34Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493161600","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493161600"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493161600"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493161600/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":69,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":69,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493162248","pull_request_review_id":493984643,"id":493162248,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE2MjI0OA==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Incomplete sentence?","created_at":"2020-09-23T02:35:50Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493162248","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493162248"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493162248"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493162248/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":81,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":81,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493163586","pull_request_review_id":493984643,"id":493163586,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE2MzU4Ng==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by\n+ * attackers. An invblocking attacker that races the network:\n+ * - Will be unsuccessful if all preferred connections are honest (and there is at least one).\n+ * - If there are P preferred connections of which Ph>=1 are honest, the attacker can delay us from learning\n+ * about a transaction by k expiration periods, where k ~ 1 + NHG(N=P-1,K=P-Ph-1,r=1), which has mean\n+ * P/(Ph+1) (where NHG stands for Negative Hypergeometric distribution).\n+ * - If all P preferred connections are to the attacker, and there are NP non-preferred connections of which NPh are\n+ * honest, k ~ P + 1 + NHG(N=NP-1,K=NP-NPh-1,r=1), with mean P + NP/(NPh+1).\n+ *\n+ * Complexity:\n+ * - Memory usage is proportional to the total number of tracked announcements (Size()) plus the number of\n+ * peers with a nonzero number of tracked announcements.\n+ * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of\n+ * announcements affected by an operation (amortized O(1) per announcement).\n+ */","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Add a reference to https://allquantor.at/blockchainbib/pdf/miller2015topology.pdf to document/explain the invblock terminology? \"What does this have to do with an INV message about a block?\"","created_at":"2020-09-23T02:41:13Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493163586","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493163586"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493163586"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493163586/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":95,"original_line":95,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":106,"position":95,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493163901","pull_request_review_id":493984643,"id":493163901,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE2MzkwMQ==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"\"and **not** being influencable by attackers\"","created_at":"2020-09-23T02:42:25Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493163901","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493163901"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493163901"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493163901/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":92,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":92,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493166644","pull_request_review_id":493984643,"id":493166644,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE2NjY0NA==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by\n+ * attackers. An invblocking attacker that races the network:\n+ * - Will be unsuccessful if all preferred connections are honest (and there is at least one).\n+ * - If there are P preferred connections of which Ph>=1 are honest, the attacker can delay us from learning\n+ * about a transaction by k expiration periods, where k ~ 1 + NHG(N=P-1,K=P-Ph-1,r=1), which has mean\n+ * P/(Ph+1) (where NHG stands for Negative Hypergeometric distribution).\n+ * - If all P preferred connections are to the attacker, and there are NP non-preferred connections of which NPh are\n+ * honest, k ~ P + 1 + NHG(N=NP-1,K=NP-NPh-1,r=1), with mean P + NP/(NPh+1).\n+ *\n+ * Complexity:\n+ * - Memory usage is proportional to the total number of tracked announcements (Size()) plus the number of\n+ * peers with a nonzero number of tracked announcements.\n+ * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of\n+ * announcements affected by an operation (amortized O(1) per announcement).\n+ */\n+class TxRequestTracker {\n+public:\n+ /** A functor with embedded salt that computes priority of a txhash/peer combination. Lower priorities are\n+ * selected first. */\n+ class PriorityComputer {\n+ const uint64_t m_k0, m_k1;\n+ public:\n+ explicit PriorityComputer(bool deterministic);\n+ uint64_t operator()(const uint256& txhash, uint64_t peer, bool preferred, bool first) const;\n+ };\n+\n+private:\n+ // Avoid littering this header file with implementation details.\n+ class Impl;\n+ std::unique_ptr m_impl;\n+\n+public:\n+ //! Construct a TxRequestTracker.\n+ TxRequestTracker(bool deterministic = false);\n+ ~TxRequestTracker();\n+\n+ // No copy constructors.\n+ TxRequestTracker(const TxRequestTracker&) = delete;\n+ TxRequestTracker& operator=(const TxRequestTracker&) = delete;\n+\n+ // Move constructors.\n+ TxRequestTracker(TxRequestTracker&&) = default;\n+ TxRequestTracker& operator=(TxRequestTracker&&) = default;\n+\n+ // Conceptually, the data structure consists of a collection of entries, one for each peer/txhash combination\n+ // (an \"announcement\"):\n+ //\n+ // - CANDIDATE entries represent transactions that were announced by a peer, and that become available for\n+ // download after their reqtime has passed.\n+ //\n+ // - REQUESTED entries represent transactions that have been requested, and which we're awaiting a response for\n+ // from that peer. Their exptime value determines when the request times out.\n+ //\n+ // - COMPLETED entries represent transactions that have been requested from a peer, but they timed out, a\n+ // NOTFOUND message was received for them, or an invalid response was received. They're only kept around to\n+ // prevent requesting them again. If only COMPLETED entries for a given txhash remain (so no CANDIDATE or\n+ // REQUESTED ones), all of them are deleted (this is an invariant, and maintained by all operations below).\n+ //\n+ // The operations below manipulate the data structure.\n+\n+ /** Deletes all entries for a given peer.\n+ *\n+ * It should be called when a peer goes offline.\n+ */\n+ void DeletedPeer(uint64_t uint64_t);","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"\"uint64_t\" as a parameter name?","created_at":"2020-09-23T02:53:21Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493166644","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493166644"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493166644"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493166644/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":156,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":156,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493168472","pull_request_review_id":493984643,"id":493168472,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE2ODQ3Mg==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by\n+ * attackers. An invblocking attacker that races the network:\n+ * - Will be unsuccessful if all preferred connections are honest (and there is at least one).\n+ * - If there are P preferred connections of which Ph>=1 are honest, the attacker can delay us from learning\n+ * about a transaction by k expiration periods, where k ~ 1 + NHG(N=P-1,K=P-Ph-1,r=1), which has mean\n+ * P/(Ph+1) (where NHG stands for Negative Hypergeometric distribution).\n+ * - If all P preferred connections are to the attacker, and there are NP non-preferred connections of which NPh are\n+ * honest, k ~ P + 1 + NHG(N=NP-1,K=NP-NPh-1,r=1), with mean P + NP/(NPh+1).\n+ *\n+ * Complexity:\n+ * - Memory usage is proportional to the total number of tracked announcements (Size()) plus the number of\n+ * peers with a nonzero number of tracked announcements.\n+ * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of\n+ * announcements affected by an operation (amortized O(1) per announcement).\n+ */\n+class TxRequestTracker {\n+public:\n+ /** A functor with embedded salt that computes priority of a txhash/peer combination. Lower priorities are\n+ * selected first. */\n+ class PriorityComputer {\n+ const uint64_t m_k0, m_k1;\n+ public:\n+ explicit PriorityComputer(bool deterministic);\n+ uint64_t operator()(const uint256& txhash, uint64_t peer, bool preferred, bool first) const;\n+ };\n+\n+private:\n+ // Avoid littering this header file with implementation details.\n+ class Impl;\n+ std::unique_ptr m_impl;","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Isn't this exactly the same performance as having a subclass and making the methods virtual, except with all the dispatching written out explicitly? ie, could instead write something like:\r\n\r\n```c++\r\nclass TxRequestTracker\r\n{\r\nprotected:\r\n TxRequestTracker() { } // pure virtual class, must instantiate via subclass\r\npublic:\r\n virtual ~TxRequestTracker();\r\n virtual void DeletedPeer(uint64_t peer) = 0;\r\n ...\r\n};\r\nstd::unique_ptr CreateTxRequestTracker(bool deterministic = false);\r\n\r\nstatic std::unique_ptr g_txrequest = CreateTxRequestTracker() GUARDED_BY(cs_main);\r\n```\r\n\r\nthen hide the subclass in txrequest.cpp? Of course, \"exactly the same\" means no objective reason to prefer changing to this.","created_at":"2020-09-23T03:00:34Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493168472","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493168472"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493168472"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493168472/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":122,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":121,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493169664","pull_request_review_id":493984643,"id":493169664,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE2OTY2NA==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by\n+ * attackers. An invblocking attacker that races the network:\n+ * - Will be unsuccessful if all preferred connections are honest (and there is at least one).\n+ * - If there are P preferred connections of which Ph>=1 are honest, the attacker can delay us from learning\n+ * about a transaction by k expiration periods, where k ~ 1 + NHG(N=P-1,K=P-Ph-1,r=1), which has mean\n+ * P/(Ph+1) (where NHG stands for Negative Hypergeometric distribution).\n+ * - If all P preferred connections are to the attacker, and there are NP non-preferred connections of which NPh are\n+ * honest, k ~ P + 1 + NHG(N=NP-1,K=NP-NPh-1,r=1), with mean P + NP/(NPh+1).\n+ *\n+ * Complexity:\n+ * - Memory usage is proportional to the total number of tracked announcements (Size()) plus the number of\n+ * peers with a nonzero number of tracked announcements.\n+ * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of\n+ * announcements affected by an operation (amortized O(1) per announcement).\n+ */\n+class TxRequestTracker {\n+public:\n+ /** A functor with embedded salt that computes priority of a txhash/peer combination. Lower priorities are\n+ * selected first. */\n+ class PriorityComputer {\n+ const uint64_t m_k0, m_k1;\n+ public:\n+ explicit PriorityComputer(bool deterministic);\n+ uint64_t operator()(const uint256& txhash, uint64_t peer, bool preferred, bool first) const;\n+ };\n+\n+private:\n+ // Avoid littering this header file with implementation details.\n+ class Impl;\n+ std::unique_ptr m_impl;\n+\n+public:\n+ //! Construct a TxRequestTracker.\n+ TxRequestTracker(bool deterministic = false);\n+ ~TxRequestTracker();\n+\n+ // No copy constructors.\n+ TxRequestTracker(const TxRequestTracker&) = delete;\n+ TxRequestTracker& operator=(const TxRequestTracker&) = delete;\n+\n+ // Move constructors.\n+ TxRequestTracker(TxRequestTracker&&) = default;\n+ TxRequestTracker& operator=(TxRequestTracker&&) = default;","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Are these needed for fuzz testing, or could they be deleted as well? (copy constructors are implicitly deleted because of the unique ptr, so I think currently this is just making the implicit defaults explicit)","created_at":"2020-09-23T03:05:34Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493169664","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493169664"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493169664"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493169664/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":134,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":134,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493172470","pull_request_review_id":494000631,"id":493172470,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE3MjQ3MA==","diff_hunk":"@@ -0,0 +1,226 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"2cac00639e5eb22eb6b076bcc001196d958a4f2e","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"I'd like to keep txrequest mostly about the decision logic and data structure, while leaving net_processing responsible for the actual policy choices (\"what delays are given to which peers/requests\", \"what timeouts are used\", \"when exactly is a peer considered preferred\", \"when exactly is a peer considered overloaded\", \"how many announcements can be tracked per peer\").\r\n\r\nOf course, the explanation here is ideally as comprehensive as possible and not full of \"See some random comment in net_processing for the details\" references. Still, do you think it would be reasonable to say instead:\r\n\r\n> Whether it's from a \"preferred\" peer or not (what that means is up to the caller, but this is designed to correspond mostly to outbound peers, or others that are more trusted)\r\n\r\n?","created_at":"2020-09-23T03:17:11Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493172470","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493172470"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493172470"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493172470/reactions","total_count":1,"+1":1,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":28,"side":"RIGHT","in_reply_to_id":492832470,"author_association":"MEMBER","original_position":28,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493177000","pull_request_review_id":493984643,"id":493177000,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE3NzAwMA==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by\n+ * attackers. An invblocking attacker that races the network:\n+ * - Will be unsuccessful if all preferred connections are honest (and there is at least one).\n+ * - If there are P preferred connections of which Ph>=1 are honest, the attacker can delay us from learning\n+ * about a transaction by k expiration periods, where k ~ 1 + NHG(N=P-1,K=P-Ph-1,r=1), which has mean\n+ * P/(Ph+1) (where NHG stands for Negative Hypergeometric distribution).\n+ * - If all P preferred connections are to the attacker, and there are NP non-preferred connections of which NPh are\n+ * honest, k ~ P + 1 + NHG(N=NP-1,K=NP-NPh-1,r=1), with mean P + NP/(NPh+1).\n+ *\n+ * Complexity:\n+ * - Memory usage is proportional to the total number of tracked announcements (Size()) plus the number of\n+ * peers with a nonzero number of tracked announcements.\n+ * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of\n+ * announcements affected by an operation (amortized O(1) per announcement).\n+ */\n+class TxRequestTracker {\n+public:\n+ /** A functor with embedded salt that computes priority of a txhash/peer combination. Lower priorities are\n+ * selected first. */\n+ class PriorityComputer {\n+ const uint64_t m_k0, m_k1;\n+ public:\n+ explicit PriorityComputer(bool deterministic);\n+ uint64_t operator()(const uint256& txhash, uint64_t peer, bool preferred, bool first) const;\n+ };\n+\n+private:\n+ // Avoid littering this header file with implementation details.\n+ class Impl;\n+ std::unique_ptr m_impl;\n+\n+public:\n+ //! Construct a TxRequestTracker.\n+ TxRequestTracker(bool deterministic = false);\n+ ~TxRequestTracker();\n+\n+ // No copy constructors.\n+ TxRequestTracker(const TxRequestTracker&) = delete;\n+ TxRequestTracker& operator=(const TxRequestTracker&) = delete;\n+\n+ // Move constructors.\n+ TxRequestTracker(TxRequestTracker&&) = default;\n+ TxRequestTracker& operator=(TxRequestTracker&&) = default;\n+\n+ // Conceptually, the data structure consists of a collection of entries, one for each peer/txhash combination\n+ // (an \"announcement\"):\n+ //\n+ // - CANDIDATE entries represent transactions that were announced by a peer, and that become available for\n+ // download after their reqtime has passed.\n+ //\n+ // - REQUESTED entries represent transactions that have been requested, and which we're awaiting a response for\n+ // from that peer. Their exptime value determines when the request times out.\n+ //\n+ // - COMPLETED entries represent transactions that have been requested from a peer, but they timed out, a\n+ // NOTFOUND message was received for them, or an invalid response was received. They're only kept around to\n+ // prevent requesting them again. If only COMPLETED entries for a given txhash remain (so no CANDIDATE or\n+ // REQUESTED ones), all of them are deleted (this is an invariant, and maintained by all operations below).\n+ //\n+ // The operations below manipulate the data structure.\n+\n+ /** Deletes all entries for a given peer.\n+ *\n+ * It should be called when a peer goes offline.\n+ */\n+ void DeletedPeer(uint64_t uint64_t);\n+\n+ /** Deletes all entries for a given txhash.\n+ *\n+ * This should be called when a transaction is successfully added to the mempool, seen in a block, or for\n+ * whatever reason we no longer care about it. The is_wtxid flag of gtxid is ignored.\n+ */\n+ void ForgetTx(const GenTxid& gtxid);\n+\n+ /** Adds a new CANDIDATE entry.\n+ *\n+ * Does nothing if one already exists for that (txhash, peer) combination (whether it's CANDIDATE, REQUESTED, or\n+ * COMPLETED). Note that this means a second INV with the same txhash from the same peer will be ignored, even\n+ * if one is a txid and the other is wtxid (but that shouldn't happen, as BIP339 requires that all announced\n+ * inventory is exclusively using MSG_WTX). The new entry is given the specified preferred and reqtime values,\n+ * and takes it is_wtxid from the specified gtxid. It is eligible to get a first marker if overloaded is false\n+ * (but also subject to the other rules regarding the first marker).\n+ */\n+ void ReceivedInv(uint64_t peer, const GenTxid& gtxid, bool preferred, bool overloaded,\n+ std::chrono::microseconds reqtime);\n+\n+ /** Converts the CANDIDATE entry for the provided peer and gtxid into a REQUESTED one.\n+ *\n+ * Exptime is set to the specified value. This can ONLY be called immediately after GetRequestable was called\n+ * (for the same peer), with only ForgetTx and other RequestedTx calls (both for other txhashes) in\n+ * between. Any other non-const operation removes the ability to call RequestedTx.\n+ */\n+ void RequestedTx(uint64_t peer, const GenTxid& gtxid, std::chrono::microseconds exptime);\n+\n+ /** Converts any CANDIDATE or REQUESTED entry to a COMPLETED one, if one exists.\n+ *\n+ * It should be called whenever a transaction or NOTFOUND was received from a peer. When a good transaction is\n+ * received, ForgetTx should be called instead of (or in addition to) this operation.\n+ */\n+ void ReceivedResponse(uint64_t peer, const GenTxid& gtxid);\n+\n+ // The operations below inspect the data structure.\n+\n+ /** Find the txids to request now from peer.\n+ *\n+ * It does the following:\n+ * - Convert all REQUESTED entries (for all txhashes/peers) with (exptime <= now) to COMPLETED entries.\n+ * - Requestable entries are selected: CANDIDATE entries from the specified peer with (reqtime <= now) for\n+ * which the specified peer is the best choice among all such CANDIDATE entries with the same txhash (subject\n+ * to preference/first rules, and tiebreaking using a deterministic salted hash of peer and txhash).\n+ * - The selected entries are sorted in order of announcement (even if multiple were added at the same time, or\n+ * even when the clock went backwards while they were being added), converted to GenTxids using their\n+ * is_wtxid flag, and returned.\n+ */\n+ std::vector GetRequestable(uint64_t peer, std::chrono::microseconds now);","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Does anything limit the size of the returned vector? I think the main constraints are those in net_processing.cpp:RequestTx which could leave as many as 100k entries in CANDIDATE state for a given peer, so this could be a 4MB vector, which seems like it might be larger than desirable?\r\n\r\nIt's also constrained by how many txs can be INVed by a peer inbetween calls to GetRequestable, so in normal circumstances I'd expect this to be perfectly fine. I think you could get the worst case by quickly sending two max-size INV messages and two max-size NOTFOUND messages once the first request comes in.\r\n\r\n(I think this has to return a copy of the data items, because you want to iterate over them and request them, which would then modify the data structure, and that would invalidate the iterator you were using if you hadn't made a copy)","created_at":"2020-09-23T03:35:42Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493177000","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493177000"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493177000"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493177000/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":173,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":205,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493179243","pull_request_review_id":493984643,"id":493179243,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE3OTI0Mw==","diff_hunk":"@@ -215,6 +215,12 @@ class TxRequestTracker {\n \n /** Access to the internal PriorityComputer (for testing) */\n const PriorityComputer& GetPriorityComputer() const;\n+\n+ /** Run internal consistency check (test only). */\n+ void SanityCheck() const;\n+\n+ /** Run a time-dependent consistency check (can only be called immediately after GetRequestable; test only). */\n+ void TimeSanityCheck(std::chrono::microseconds now) const;","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"c50b25c2c4c63e1cfea6428913f35926282f3fd8","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"\"Run a check on consistency of request times after a call to GetRequestable (requires the same timestamp as was passed to GetRequestable)\" might be a better description? (At first glance I thought it might have been timing the sanity check, or doing a limited sanity check that checked different things depending on how much time it was taking to run)","created_at":"2020-09-23T03:44:42Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493179243","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493179243"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493179243"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493179243/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":223,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":9,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493187263","pull_request_review_id":493984643,"id":493187263,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE4NzI2Mw==","diff_hunk":"@@ -793,69 +726,24 @@ static void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vec\n }\n }\n \n-void EraseTxRequest(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- g_already_asked_for.erase(gtxid.GetHash());\n-}\n-\n-std::chrono::microseconds GetTxRequestTime(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- auto it = g_already_asked_for.find(gtxid.GetHash());\n- if (it != g_already_asked_for.end()) {\n- return it->second;\n- }\n- return {};\n-}\n-\n-void UpdateTxRequestTime(const GenTxid& gtxid, std::chrono::microseconds request_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- auto it = g_already_asked_for.find(gtxid.GetHash());\n- if (it == g_already_asked_for.end()) {\n- g_already_asked_for.insert(std::make_pair(gtxid.GetHash(), request_time));\n- } else {\n- g_already_asked_for.update(it, request_time);\n- }\n-}\n-\n-std::chrono::microseconds CalculateTxGetDataTime(const GenTxid& gtxid, std::chrono::microseconds current_time, bool use_inbound_delay, bool use_txid_delay) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n+void RequestTx(NodeId nodeid, const GenTxid& gtxid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n {\n- std::chrono::microseconds process_time;\n- const auto last_request_time = GetTxRequestTime(gtxid);\n- // First time requesting this tx\n- if (last_request_time.count() == 0) {\n- process_time = current_time;\n- } else {\n- // Randomize the delay to avoid biasing some peers over others (such as due to\n- // fixed ordering of peer processing in ThreadMessageHandler)\n- process_time = last_request_time + GETDATA_TX_INTERVAL + GetRandMicros(MAX_GETDATA_RANDOM_DELAY);\n- }\n-\n- // We delay processing announcements from inbound peers\n- if (use_inbound_delay) process_time += INBOUND_PEER_TX_DELAY;\n-\n- // We delay processing announcements from peers that use txid-relay (instead of wtxid)\n- if (use_txid_delay) process_time += TXID_RELAY_DELAY;\n-\n- return process_time;\n-}\n-\n-void RequestTx(CNodeState* state, const GenTxid& gtxid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- CNodeState::TxDownloadState& peer_download_state = state->m_tx_download;\n- if (peer_download_state.m_tx_announced.size() >= MAX_PEER_TX_ANNOUNCEMENTS ||\n- peer_download_state.m_tx_process_time.size() >= MAX_PEER_TX_ANNOUNCEMENTS ||\n- peer_download_state.m_tx_announced.count(gtxid.GetHash())) {\n- // Too many queued announcements from this peer, or we already have\n- // this announcement\n+ if (g_txrequest.CountTracked(nodeid) >= MAX_PEER_TX_ANNOUNCEMENTS) {\n+ // Too many queued announcements from this peer\n return;\n }\n- peer_download_state.m_tx_announced.insert(gtxid.GetHash());\n-\n- // Calculate the time to try requesting this transaction. Use\n- // fPreferredDownload as a proxy for outbound peers.\n- const auto process_time = CalculateTxGetDataTime(gtxid, current_time, !state->fPreferredDownload, !state->m_wtxid_relay && g_wtxid_relay_peers > 0);\n-\n- peer_download_state.m_tx_process_time.emplace(process_time, gtxid);\n+ auto state = State(nodeid);\n+ auto delay = std::chrono::microseconds{0};\n+ bool preferred = state->fPreferredDownload;\n+ bool overloaded = g_txrequest.CountInFlight(nodeid) >= MAX_PEER_TX_IN_FLIGHT;","path":"src/net_processing.cpp","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"c7601cdb31f9c86a4308cf36bbbc9c74c1078f53","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Maybe this should be `overloaded == ... || g_txrequest.CountCandidates() >= 5000` or similar? I'm not sure I have a plausible enough scenario where this would be a benefit to justify the added code though.","created_at":"2020-09-23T04:18:11Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493187263","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493187263"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493187263"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493187263/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":747,"side":"RIGHT","author_association":"CONTRIBUTOR","original_position":178,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493190576","pull_request_review_id":493984643,"id":493190576,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE5MDU3Ng==","diff_hunk":"@@ -793,69 +726,24 @@ static void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vec\n }\n }\n \n-void EraseTxRequest(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- g_already_asked_for.erase(gtxid.GetHash());\n-}\n-\n-std::chrono::microseconds GetTxRequestTime(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- auto it = g_already_asked_for.find(gtxid.GetHash());\n- if (it != g_already_asked_for.end()) {\n- return it->second;\n- }\n- return {};\n-}\n-\n-void UpdateTxRequestTime(const GenTxid& gtxid, std::chrono::microseconds request_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- auto it = g_already_asked_for.find(gtxid.GetHash());\n- if (it == g_already_asked_for.end()) {\n- g_already_asked_for.insert(std::make_pair(gtxid.GetHash(), request_time));\n- } else {\n- g_already_asked_for.update(it, request_time);\n- }\n-}\n-\n-std::chrono::microseconds CalculateTxGetDataTime(const GenTxid& gtxid, std::chrono::microseconds current_time, bool use_inbound_delay, bool use_txid_delay) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n+void RequestTx(NodeId nodeid, const GenTxid& gtxid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n {\n- std::chrono::microseconds process_time;\n- const auto last_request_time = GetTxRequestTime(gtxid);\n- // First time requesting this tx\n- if (last_request_time.count() == 0) {\n- process_time = current_time;\n- } else {\n- // Randomize the delay to avoid biasing some peers over others (such as due to\n- // fixed ordering of peer processing in ThreadMessageHandler)\n- process_time = last_request_time + GETDATA_TX_INTERVAL + GetRandMicros(MAX_GETDATA_RANDOM_DELAY);\n- }\n-\n- // We delay processing announcements from inbound peers\n- if (use_inbound_delay) process_time += INBOUND_PEER_TX_DELAY;\n-\n- // We delay processing announcements from peers that use txid-relay (instead of wtxid)\n- if (use_txid_delay) process_time += TXID_RELAY_DELAY;\n-\n- return process_time;\n-}\n-\n-void RequestTx(CNodeState* state, const GenTxid& gtxid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)","path":"src/net_processing.cpp","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"c7601cdb31f9c86a4308cf36bbbc9c74c1078f53","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"I think if you were to make `RequestTx()` a method of `class PeerManager`, you could make `g_txrequest` be a private member `PeerManager::m_txrequest GUARDED_BY(cs_main)` instead of a global. OTOH, might be better to not do that until other globals that `PeerManager` methods rely on (like `mapNodeState`) are also moved. cc @jnewbery","created_at":"2020-09-23T04:30:45Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493190576","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493190576"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493190576"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493190576/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":866,"original_line":866,"side":"LEFT","author_association":"CONTRIBUTOR","original_position":156,"position":168,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493198222","pull_request_review_id":493984643,"id":493198222,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzE5ODIyMg==","diff_hunk":"@@ -121,38 +116,10 @@ def test_inv_block(self):\n # * the first time it is re-requested from the outbound peer, plus\n # * 2 seconds to avoid races\n assert self.nodes[1].getpeerinfo()[0]['inbound'] == False\n- timeout = 2 + (MAX_GETDATA_RANDOM_DELAY + INBOUND_PEER_TX_DELAY) + (\n- GETDATA_TX_INTERVAL + MAX_GETDATA_RANDOM_DELAY)\n+ timeout = 2 + INBOUND_PEER_TX_DELAY + GETDATA_TX_INTERVAL\n self.log.info(\"Tx should be received at node 1 after {} seconds\".format(timeout))\n self.sync_mempools(timeout=timeout)\n \n- def test_in_flight_max(self):","path":"test/functional/p2p_tx_download.py","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"c7601cdb31f9c86a4308cf36bbbc9c74c1078f53","user":{"login":"ajtowns","id":127186,"node_id":"MDQ6VXNlcjEyNzE4Ng==","avatar_url":"https://avatars.githubusercontent.com/u/127186?v=4","gravatar_id":"","url":"https://api.github.com/users/ajtowns","html_url":"https://github.com/ajtowns","followers_url":"https://api.github.com/users/ajtowns/followers","following_url":"https://api.github.com/users/ajtowns/following{/other_user}","gists_url":"https://api.github.com/users/ajtowns/gists{/gist_id}","starred_url":"https://api.github.com/users/ajtowns/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ajtowns/subscriptions","organizations_url":"https://api.github.com/users/ajtowns/orgs","repos_url":"https://api.github.com/users/ajtowns/repos","events_url":"https://api.github.com/users/ajtowns/events{/privacy}","received_events_url":"https://api.github.com/users/ajtowns/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Should replace this test with one that checks we start applying `OVERLOAD_PEER_TX_DELAY` ?","created_at":"2020-09-23T05:00:55Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493198222","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493198222"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493198222"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493198222/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":129,"original_line":129,"side":"LEFT","author_association":"CONTRIBUTOR","original_position":37,"position":29,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493830506","pull_request_review_id":494965962,"id":493830506,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5MzgzMDUwNg==","diff_hunk":"@@ -121,38 +116,10 @@ def test_inv_block(self):\n # * the first time it is re-requested from the outbound peer, plus\n # * 2 seconds to avoid races\n assert self.nodes[1].getpeerinfo()[0]['inbound'] == False\n- timeout = 2 + (MAX_GETDATA_RANDOM_DELAY + INBOUND_PEER_TX_DELAY) + (\n- GETDATA_TX_INTERVAL + MAX_GETDATA_RANDOM_DELAY)\n+ timeout = 2 + INBOUND_PEER_TX_DELAY + GETDATA_TX_INTERVAL\n self.log.info(\"Tx should be received at node 1 after {} seconds\".format(timeout))\n self.sync_mempools(timeout=timeout)\n \n- def test_in_flight_max(self):","path":"test/functional/p2p_tx_download.py","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"c7601cdb31f9c86a4308cf36bbbc9c74c1078f53","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"@ajtowns Feel like writing such a test?","created_at":"2020-09-23T19:05:24Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493830506","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493830506"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493830506"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493830506/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":129,"original_line":129,"side":"LEFT","in_reply_to_id":493198222,"author_association":"MEMBER","original_position":37,"position":29,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966435","pull_request_review_id":495137408,"id":493966435,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2NjQzNQ==","diff_hunk":"@@ -0,0 +1,226 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"2cac00639e5eb22eb6b076bcc001196d958a4f2e","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Done, approximately.","created_at":"2020-09-24T00:10:18Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966435","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966435"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966435"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966435/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":28,"side":"RIGHT","in_reply_to_id":492832470,"author_association":"MEMBER","original_position":28,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966501","pull_request_review_id":495137464,"id":493966501,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2NjUwMQ==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Changed to \"expiry\".","created_at":"2020-09-24T00:10:29Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966501","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966501"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966501"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966501/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":30,"side":"RIGHT","in_reply_to_id":493158786,"author_association":"MEMBER","original_position":30,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966652","pull_request_review_id":495137619,"id":493966652,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2NjY1Mg==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"I was trying to refer to the specific section on the \"first\" marker. Made that more explicit.","created_at":"2020-09-24T00:10:57Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966652","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966652"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966652"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966652/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":29,"side":"RIGHT","in_reply_to_id":493160848,"author_association":"MEMBER","original_position":29,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966692","pull_request_review_id":495137670,"id":493966692,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2NjY5Mg==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Done.","created_at":"2020-09-24T00:11:05Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966692","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966692"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966692"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966692/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":69,"side":"RIGHT","in_reply_to_id":493161600,"author_association":"MEMBER","original_position":69,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966778","pull_request_review_id":495137767,"id":493966778,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2Njc3OA==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Removed. It was a leftover of what turned into the last comment section.","created_at":"2020-09-24T00:11:20Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966778","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966778"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966778"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966778/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":81,"side":"RIGHT","in_reply_to_id":493162248,"author_association":"MEMBER","original_position":81,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966818","pull_request_review_id":495137808,"id":493966818,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2NjgxOA==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by\n+ * attackers. An invblocking attacker that races the network:\n+ * - Will be unsuccessful if all preferred connections are honest (and there is at least one).\n+ * - If there are P preferred connections of which Ph>=1 are honest, the attacker can delay us from learning\n+ * about a transaction by k expiration periods, where k ~ 1 + NHG(N=P-1,K=P-Ph-1,r=1), which has mean\n+ * P/(Ph+1) (where NHG stands for Negative Hypergeometric distribution).\n+ * - If all P preferred connections are to the attacker, and there are NP non-preferred connections of which NPh are\n+ * honest, k ~ P + 1 + NHG(N=NP-1,K=NP-NPh-1,r=1), with mean P + NP/(NPh+1).\n+ *\n+ * Complexity:\n+ * - Memory usage is proportional to the total number of tracked announcements (Size()) plus the number of\n+ * peers with a nonzero number of tracked announcements.\n+ * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of\n+ * announcements affected by an operation (amortized O(1) per announcement).\n+ */","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Done.","created_at":"2020-09-24T00:11:26Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966818","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966818"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966818"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966818/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":95,"original_line":95,"side":"RIGHT","in_reply_to_id":493163586,"author_association":"MEMBER","original_position":106,"position":95,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966899","pull_request_review_id":495137901,"id":493966899,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2Njg5OQ==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Done, approximately.","created_at":"2020-09-24T00:11:41Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966899","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966899"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493966899"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493966899/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":92,"side":"RIGHT","in_reply_to_id":493163901,"author_association":"MEMBER","original_position":92,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493967057","pull_request_review_id":495138090,"id":493967057,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2NzA1Nw==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by\n+ * attackers. An invblocking attacker that races the network:\n+ * - Will be unsuccessful if all preferred connections are honest (and there is at least one).\n+ * - If there are P preferred connections of which Ph>=1 are honest, the attacker can delay us from learning\n+ * about a transaction by k expiration periods, where k ~ 1 + NHG(N=P-1,K=P-Ph-1,r=1), which has mean\n+ * P/(Ph+1) (where NHG stands for Negative Hypergeometric distribution).\n+ * - If all P preferred connections are to the attacker, and there are NP non-preferred connections of which NPh are\n+ * honest, k ~ P + 1 + NHG(N=NP-1,K=NP-NPh-1,r=1), with mean P + NP/(NPh+1).\n+ *\n+ * Complexity:\n+ * - Memory usage is proportional to the total number of tracked announcements (Size()) plus the number of\n+ * peers with a nonzero number of tracked announcements.\n+ * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of\n+ * announcements affected by an operation (amortized O(1) per announcement).\n+ */\n+class TxRequestTracker {\n+public:\n+ /** A functor with embedded salt that computes priority of a txhash/peer combination. Lower priorities are\n+ * selected first. */\n+ class PriorityComputer {\n+ const uint64_t m_k0, m_k1;\n+ public:\n+ explicit PriorityComputer(bool deterministic);\n+ uint64_t operator()(const uint256& txhash, uint64_t peer, bool preferred, bool first) const;\n+ };\n+\n+private:\n+ // Avoid littering this header file with implementation details.\n+ class Impl;\n+ std::unique_ptr m_impl;\n+\n+public:\n+ //! Construct a TxRequestTracker.\n+ TxRequestTracker(bool deterministic = false);\n+ ~TxRequestTracker();\n+\n+ // No copy constructors.\n+ TxRequestTracker(const TxRequestTracker&) = delete;\n+ TxRequestTracker& operator=(const TxRequestTracker&) = delete;\n+\n+ // Move constructors.\n+ TxRequestTracker(TxRequestTracker&&) = default;\n+ TxRequestTracker& operator=(TxRequestTracker&&) = default;\n+\n+ // Conceptually, the data structure consists of a collection of entries, one for each peer/txhash combination\n+ // (an \"announcement\"):\n+ //\n+ // - CANDIDATE entries represent transactions that were announced by a peer, and that become available for\n+ // download after their reqtime has passed.\n+ //\n+ // - REQUESTED entries represent transactions that have been requested, and which we're awaiting a response for\n+ // from that peer. Their exptime value determines when the request times out.\n+ //\n+ // - COMPLETED entries represent transactions that have been requested from a peer, but they timed out, a\n+ // NOTFOUND message was received for them, or an invalid response was received. They're only kept around to\n+ // prevent requesting them again. If only COMPLETED entries for a given txhash remain (so no CANDIDATE or\n+ // REQUESTED ones), all of them are deleted (this is an invariant, and maintained by all operations below).\n+ //\n+ // The operations below manipulate the data structure.\n+\n+ /** Deletes all entries for a given peer.\n+ *\n+ * It should be called when a peer goes offline.\n+ */\n+ void DeletedPeer(uint64_t uint64_t);","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Apparently that's not actually a 128-bit type ;)\r\n\r\nFixed.","created_at":"2020-09-24T00:12:19Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493967057","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493967057"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493967057"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493967057/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":156,"side":"RIGHT","in_reply_to_id":493166644,"author_association":"MEMBER","original_position":156,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493968967","pull_request_review_id":495140122,"id":493968967,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2ODk2Nw==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by\n+ * attackers. An invblocking attacker that races the network:\n+ * - Will be unsuccessful if all preferred connections are honest (and there is at least one).\n+ * - If there are P preferred connections of which Ph>=1 are honest, the attacker can delay us from learning\n+ * about a transaction by k expiration periods, where k ~ 1 + NHG(N=P-1,K=P-Ph-1,r=1), which has mean\n+ * P/(Ph+1) (where NHG stands for Negative Hypergeometric distribution).\n+ * - If all P preferred connections are to the attacker, and there are NP non-preferred connections of which NPh are\n+ * honest, k ~ P + 1 + NHG(N=NP-1,K=NP-NPh-1,r=1), with mean P + NP/(NPh+1).\n+ *\n+ * Complexity:\n+ * - Memory usage is proportional to the total number of tracked announcements (Size()) plus the number of\n+ * peers with a nonzero number of tracked announcements.\n+ * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of\n+ * announcements affected by an operation (amortized O(1) per announcement).\n+ */\n+class TxRequestTracker {\n+public:\n+ /** A functor with embedded salt that computes priority of a txhash/peer combination. Lower priorities are\n+ * selected first. */\n+ class PriorityComputer {\n+ const uint64_t m_k0, m_k1;\n+ public:\n+ explicit PriorityComputer(bool deterministic);\n+ uint64_t operator()(const uint256& txhash, uint64_t peer, bool preferred, bool first) const;\n+ };\n+\n+private:\n+ // Avoid littering this header file with implementation details.\n+ class Impl;\n+ std::unique_ptr m_impl;","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"It's not **exactly** the same, I think. Calling member functions on a `TxRequestTracker` would incur a lookup in the vtable to find the code to execute. The current solution has link-time determined code flow, and only an extra indirection to find the object's storage.\r\n\r\nI don't think either is remotely relevant for performance here, and the subclass approach you suggest is probably somewhat lighter syntactically. I'm happy to change it if other reviewers agree.","created_at":"2020-09-24T00:18:55Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493968967","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493968967"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493968967"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493968967/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":122,"side":"RIGHT","in_reply_to_id":493168472,"author_association":"MEMBER","original_position":121,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493969150","pull_request_review_id":495140308,"id":493969150,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2OTE1MA==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by\n+ * attackers. An invblocking attacker that races the network:\n+ * - Will be unsuccessful if all preferred connections are honest (and there is at least one).\n+ * - If there are P preferred connections of which Ph>=1 are honest, the attacker can delay us from learning\n+ * about a transaction by k expiration periods, where k ~ 1 + NHG(N=P-1,K=P-Ph-1,r=1), which has mean\n+ * P/(Ph+1) (where NHG stands for Negative Hypergeometric distribution).\n+ * - If all P preferred connections are to the attacker, and there are NP non-preferred connections of which NPh are\n+ * honest, k ~ P + 1 + NHG(N=NP-1,K=NP-NPh-1,r=1), with mean P + NP/(NPh+1).\n+ *\n+ * Complexity:\n+ * - Memory usage is proportional to the total number of tracked announcements (Size()) plus the number of\n+ * peers with a nonzero number of tracked announcements.\n+ * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of\n+ * announcements affected by an operation (amortized O(1) per announcement).\n+ */\n+class TxRequestTracker {\n+public:\n+ /** A functor with embedded salt that computes priority of a txhash/peer combination. Lower priorities are\n+ * selected first. */\n+ class PriorityComputer {\n+ const uint64_t m_k0, m_k1;\n+ public:\n+ explicit PriorityComputer(bool deterministic);\n+ uint64_t operator()(const uint256& txhash, uint64_t peer, bool preferred, bool first) const;\n+ };\n+\n+private:\n+ // Avoid littering this header file with implementation details.\n+ class Impl;\n+ std::unique_ptr m_impl;\n+\n+public:\n+ //! Construct a TxRequestTracker.\n+ TxRequestTracker(bool deterministic = false);\n+ ~TxRequestTracker();\n+\n+ // No copy constructors.\n+ TxRequestTracker(const TxRequestTracker&) = delete;\n+ TxRequestTracker& operator=(const TxRequestTracker&) = delete;\n+\n+ // Move constructors.\n+ TxRequestTracker(TxRequestTracker&&) = default;\n+ TxRequestTracker& operator=(TxRequestTracker&&) = default;","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Gone.","created_at":"2020-09-24T00:19:27Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493969150","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493969150"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493969150"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493969150/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":134,"side":"RIGHT","in_reply_to_id":493169664,"author_association":"MEMBER","original_position":134,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493969647","pull_request_review_id":495140823,"id":493969647,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk2OTY0Nw==","diff_hunk":"@@ -0,0 +1,220 @@\n+// Copyright (c) 2020 The Bitcoin Core developers\n+// Distributed under the MIT software license, see the accompanying\n+// file COPYING or http://www.opensource.org/licenses/mit-license.php.\n+\n+#ifndef BITCOIN_TXREQUEST_H\n+#define BITCOIN_TXREQUEST_H\n+\n+#include \n+#include \n+\n+#include \n+#include \n+\n+#include \n+\n+/** Data structure to keep track of, and schedule, transaction downloads from peers.\n+ *\n+ * === Specification ===\n+ *\n+ * We keep track of which peers have announced which transactions, and use that to determine which requests\n+ * should go to which peer, when, and in what order.\n+ *\n+ * The following information is tracked per peer/tx combination (\"announcement\"):\n+ * - Which peer announced it (through their NodeId)\n+ * - The txid or wtxid of the transaction (collectively called \"txhash\" in what follows)\n+ * - Whether it was a tx or wtx announcement (see BIP339).\n+ * - What the earliest permitted time is that that transaction can be requested from that peer (called \"reqtime\").\n+ * - Whether it's from a \"preferred\" peer or not (outbound and whitelisted peers are preferred).\n+ * - Whether the peer was the \"first\" to announce this txhash within its class (see further for details).\n+ * - Whether or not the transaction was requested already, and if so, when it times out (called \"exptime\").\n+ * - Whether or not the transaction request failed already (timed out, or NOTFOUND was received).\n+ *\n+ * Transaction requests are then assigned to peers, following these rules:\n+ *\n+ * - No transaction is requested as long as another request for the same txhash is outstanding (it needs to fail\n+ * first by passing exptime, or a NOTFOUND or invalid transaction has to be received for it).\n+ *\n+ * Rationale: to avoid wasting bandwidth on multiple copies of the same transaction.\n+ *\n+ * - The same transaction is never requested twice from the same peer, unless the transaction was forgotten in\n+ * between (see next point), and re-announced.\n+ *\n+ * Rationale: giving a peer multiple chances to announce a transaction would allow them to bias requests in their\n+ * favor, worsening invblock attacks. The flip side is that as long as an attacker manages to prevent\n+ * us from receiving a transaction, failed announcements (including those from honest peers) will\n+ * linger longer, increasing memory usage somewhat. The impact of this is limited by imposing a cap on\n+ * the number of tracked announcements per peer.\n+ *\n+ * - Announcements are only forgotten about when the peer that announced them went offline, when the transaction\n+ * was received successfully, or when no candidates for a transaction remain that haven't been tried already.\n+ *\n+ * Rationale: we need to eventually forget announcements to keep memory bounded, but as long as viable\n+ * candidate peers remain, we prefer to avoid fetching from failed ones. As every request has a finite\n+ * timeout and we schedule new request as soon a previous one expired, there is always progress being\n+ * made towards forgetting a transaction - either successfully or unsuccessfully.\n+ *\n+ * - Transactions are not requested from a peer until its reqtime has passed.\n+ *\n+ * Rationale: enable net_processing code to define a delay for less-than-ideal peers, so that (presumed) better\n+ * peers have a chance to give their announcement first.\n+ *\n+ * - If multiple viable candidate peers exist according to the above rules, pick a peer as follows:\n+ *\n+ * - If any preferred peers are available, non-preferred peers are not considered for what follows.\n+ *\n+ * Rationale: preferred peers (outbound, whitelisted) are chosen by us, so are less likely to be under attacker\n+ * control.\n+ *\n+ * - Among the remaining candidates, choose the one with the first marker if one exists (there can be at most\n+ * one such peer, see further).\n+ *\n+ * Rationale: in non-attack scenarios we want to give one chance to request from the fastest peer to reduce\n+ * latency, and reduce risk of breaking chains of dependent transactions. An attacker who races the\n+ * network can exploit this to delay us learning about a transaction, but it is available only once\n+ * per txhash.\n+ *\n+ * - If no remaining candidates have the first marker, pick a uniformly random peer among the candidates.\n+ *\n+ * Rationale: if the first mechanism failed, random assignments are hard to influence for attackers.\n+ *\n+ * The overall effect of these rules is that an attacker that races announcements to us can delay\n+ *\n+ * \"First\" marker rules: the first marker is given to announcements for which at the time of announcement:\n+ * - No requests for its txhash have been attempted (ever, or since it was forgotten about).\n+ * - The peer that announced them was not overloaded.\n+ * - No announcement for the same txhash from another peer within the same preferred/nonpreferred class already has\n+ * the first marker already.\n+ *\n+ * Rationale: these restrictions avoid giving the speed benefit to honest but overloaded peers, and also\n+ * reduce the impact an attacker who races the network for many transaction can have.\n+ *\n+ * Together these rules strike a balance between being fast in non-adverserial conditions and being influenceable by\n+ * attackers. An invblocking attacker that races the network:\n+ * - Will be unsuccessful if all preferred connections are honest (and there is at least one).\n+ * - If there are P preferred connections of which Ph>=1 are honest, the attacker can delay us from learning\n+ * about a transaction by k expiration periods, where k ~ 1 + NHG(N=P-1,K=P-Ph-1,r=1), which has mean\n+ * P/(Ph+1) (where NHG stands for Negative Hypergeometric distribution).\n+ * - If all P preferred connections are to the attacker, and there are NP non-preferred connections of which NPh are\n+ * honest, k ~ P + 1 + NHG(N=NP-1,K=NP-NPh-1,r=1), with mean P + NP/(NPh+1).\n+ *\n+ * Complexity:\n+ * - Memory usage is proportional to the total number of tracked announcements (Size()) plus the number of\n+ * peers with a nonzero number of tracked announcements.\n+ * - CPU usage is generally logarithmic in the total number of tracked announcements, plus the number of\n+ * announcements affected by an operation (amortized O(1) per announcement).\n+ */\n+class TxRequestTracker {\n+public:\n+ /** A functor with embedded salt that computes priority of a txhash/peer combination. Lower priorities are\n+ * selected first. */\n+ class PriorityComputer {\n+ const uint64_t m_k0, m_k1;\n+ public:\n+ explicit PriorityComputer(bool deterministic);\n+ uint64_t operator()(const uint256& txhash, uint64_t peer, bool preferred, bool first) const;\n+ };\n+\n+private:\n+ // Avoid littering this header file with implementation details.\n+ class Impl;\n+ std::unique_ptr m_impl;\n+\n+public:\n+ //! Construct a TxRequestTracker.\n+ TxRequestTracker(bool deterministic = false);\n+ ~TxRequestTracker();\n+\n+ // No copy constructors.\n+ TxRequestTracker(const TxRequestTracker&) = delete;\n+ TxRequestTracker& operator=(const TxRequestTracker&) = delete;\n+\n+ // Move constructors.\n+ TxRequestTracker(TxRequestTracker&&) = default;\n+ TxRequestTracker& operator=(TxRequestTracker&&) = default;\n+\n+ // Conceptually, the data structure consists of a collection of entries, one for each peer/txhash combination\n+ // (an \"announcement\"):\n+ //\n+ // - CANDIDATE entries represent transactions that were announced by a peer, and that become available for\n+ // download after their reqtime has passed.\n+ //\n+ // - REQUESTED entries represent transactions that have been requested, and which we're awaiting a response for\n+ // from that peer. Their exptime value determines when the request times out.\n+ //\n+ // - COMPLETED entries represent transactions that have been requested from a peer, but they timed out, a\n+ // NOTFOUND message was received for them, or an invalid response was received. They're only kept around to\n+ // prevent requesting them again. If only COMPLETED entries for a given txhash remain (so no CANDIDATE or\n+ // REQUESTED ones), all of them are deleted (this is an invariant, and maintained by all operations below).\n+ //\n+ // The operations below manipulate the data structure.\n+\n+ /** Deletes all entries for a given peer.\n+ *\n+ * It should be called when a peer goes offline.\n+ */\n+ void DeletedPeer(uint64_t uint64_t);\n+\n+ /** Deletes all entries for a given txhash.\n+ *\n+ * This should be called when a transaction is successfully added to the mempool, seen in a block, or for\n+ * whatever reason we no longer care about it. The is_wtxid flag of gtxid is ignored.\n+ */\n+ void ForgetTx(const GenTxid& gtxid);\n+\n+ /** Adds a new CANDIDATE entry.\n+ *\n+ * Does nothing if one already exists for that (txhash, peer) combination (whether it's CANDIDATE, REQUESTED, or\n+ * COMPLETED). Note that this means a second INV with the same txhash from the same peer will be ignored, even\n+ * if one is a txid and the other is wtxid (but that shouldn't happen, as BIP339 requires that all announced\n+ * inventory is exclusively using MSG_WTX). The new entry is given the specified preferred and reqtime values,\n+ * and takes it is_wtxid from the specified gtxid. It is eligible to get a first marker if overloaded is false\n+ * (but also subject to the other rules regarding the first marker).\n+ */\n+ void ReceivedInv(uint64_t peer, const GenTxid& gtxid, bool preferred, bool overloaded,\n+ std::chrono::microseconds reqtime);\n+\n+ /** Converts the CANDIDATE entry for the provided peer and gtxid into a REQUESTED one.\n+ *\n+ * Exptime is set to the specified value. This can ONLY be called immediately after GetRequestable was called\n+ * (for the same peer), with only ForgetTx and other RequestedTx calls (both for other txhashes) in\n+ * between. Any other non-const operation removes the ability to call RequestedTx.\n+ */\n+ void RequestedTx(uint64_t peer, const GenTxid& gtxid, std::chrono::microseconds exptime);\n+\n+ /** Converts any CANDIDATE or REQUESTED entry to a COMPLETED one, if one exists.\n+ *\n+ * It should be called whenever a transaction or NOTFOUND was received from a peer. When a good transaction is\n+ * received, ForgetTx should be called instead of (or in addition to) this operation.\n+ */\n+ void ReceivedResponse(uint64_t peer, const GenTxid& gtxid);\n+\n+ // The operations below inspect the data structure.\n+\n+ /** Find the txids to request now from peer.\n+ *\n+ * It does the following:\n+ * - Convert all REQUESTED entries (for all txhashes/peers) with (exptime <= now) to COMPLETED entries.\n+ * - Requestable entries are selected: CANDIDATE entries from the specified peer with (reqtime <= now) for\n+ * which the specified peer is the best choice among all such CANDIDATE entries with the same txhash (subject\n+ * to preference/first rules, and tiebreaking using a deterministic salted hash of peer and txhash).\n+ * - The selected entries are sorted in order of announcement (even if multiple were added at the same time, or\n+ * even when the clock went backwards while they were being added), converted to GenTxids using their\n+ * is_wtxid flag, and returned.\n+ */\n+ std::vector GetRequestable(uint64_t peer, std::chrono::microseconds now);","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"1b23bedf579aa615fa27a114da7cefbee756af6d","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"I've added a commit that just reduces MAX_PEER_TX_ANNOUNCEMENTS; 100000 was ridiculous. It can be bypassed using the PF_RELAY permission.","created_at":"2020-09-24T00:21:04Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493969647","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493969647"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493969647"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493969647/reactions","total_count":1,"+1":1,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":173,"side":"RIGHT","in_reply_to_id":493177000,"author_association":"MEMBER","original_position":205,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493970079","pull_request_review_id":495141294,"id":493970079,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk3MDA3OQ==","diff_hunk":"@@ -793,69 +726,24 @@ static void FindNextBlocksToDownload(NodeId nodeid, unsigned int count, std::vec\n }\n }\n \n-void EraseTxRequest(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- g_already_asked_for.erase(gtxid.GetHash());\n-}\n-\n-std::chrono::microseconds GetTxRequestTime(const GenTxid& gtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- auto it = g_already_asked_for.find(gtxid.GetHash());\n- if (it != g_already_asked_for.end()) {\n- return it->second;\n- }\n- return {};\n-}\n-\n-void UpdateTxRequestTime(const GenTxid& gtxid, std::chrono::microseconds request_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- auto it = g_already_asked_for.find(gtxid.GetHash());\n- if (it == g_already_asked_for.end()) {\n- g_already_asked_for.insert(std::make_pair(gtxid.GetHash(), request_time));\n- } else {\n- g_already_asked_for.update(it, request_time);\n- }\n-}\n-\n-std::chrono::microseconds CalculateTxGetDataTime(const GenTxid& gtxid, std::chrono::microseconds current_time, bool use_inbound_delay, bool use_txid_delay) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n+void RequestTx(NodeId nodeid, const GenTxid& gtxid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n {\n- std::chrono::microseconds process_time;\n- const auto last_request_time = GetTxRequestTime(gtxid);\n- // First time requesting this tx\n- if (last_request_time.count() == 0) {\n- process_time = current_time;\n- } else {\n- // Randomize the delay to avoid biasing some peers over others (such as due to\n- // fixed ordering of peer processing in ThreadMessageHandler)\n- process_time = last_request_time + GETDATA_TX_INTERVAL + GetRandMicros(MAX_GETDATA_RANDOM_DELAY);\n- }\n-\n- // We delay processing announcements from inbound peers\n- if (use_inbound_delay) process_time += INBOUND_PEER_TX_DELAY;\n-\n- // We delay processing announcements from peers that use txid-relay (instead of wtxid)\n- if (use_txid_delay) process_time += TXID_RELAY_DELAY;\n-\n- return process_time;\n-}\n-\n-void RequestTx(CNodeState* state, const GenTxid& gtxid, std::chrono::microseconds current_time) EXCLUSIVE_LOCKS_REQUIRED(cs_main)\n-{\n- CNodeState::TxDownloadState& peer_download_state = state->m_tx_download;\n- if (peer_download_state.m_tx_announced.size() >= MAX_PEER_TX_ANNOUNCEMENTS ||\n- peer_download_state.m_tx_process_time.size() >= MAX_PEER_TX_ANNOUNCEMENTS ||\n- peer_download_state.m_tx_announced.count(gtxid.GetHash())) {\n- // Too many queued announcements from this peer, or we already have\n- // this announcement\n+ if (g_txrequest.CountTracked(nodeid) >= MAX_PEER_TX_ANNOUNCEMENTS) {\n+ // Too many queued announcements from this peer\n return;\n }\n- peer_download_state.m_tx_announced.insert(gtxid.GetHash());\n-\n- // Calculate the time to try requesting this transaction. Use\n- // fPreferredDownload as a proxy for outbound peers.\n- const auto process_time = CalculateTxGetDataTime(gtxid, current_time, !state->fPreferredDownload, !state->m_wtxid_relay && g_wtxid_relay_peers > 0);\n-\n- peer_download_state.m_tx_process_time.emplace(process_time, gtxid);\n+ auto state = State(nodeid);\n+ auto delay = std::chrono::microseconds{0};\n+ bool preferred = state->fPreferredDownload;\n+ bool overloaded = g_txrequest.CountInFlight(nodeid) >= MAX_PEER_TX_IN_FLIGHT;","path":"src/net_processing.cpp","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"c7601cdb31f9c86a4308cf36bbbc9c74c1078f53","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"With MAX_PEER_TX_ANNOUNCEMENTS reduced, is that still needed?\r\n\r\nI'm a bit hesitant about this, as the number of announcements tracked for a peer is somewhat dependent on other peers' behavior.","created_at":"2020-09-24T00:22:26Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493970079","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493970079"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493970079"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493970079/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":747,"side":"RIGHT","in_reply_to_id":493187263,"author_association":"MEMBER","original_position":178,"position":1,"subject_type":"line"},{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493970217","pull_request_review_id":495141467,"id":493970217,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDQ5Mzk3MDIxNw==","diff_hunk":"@@ -215,6 +215,12 @@ class TxRequestTracker {\n \n /** Access to the internal PriorityComputer (for testing) */\n const PriorityComputer& GetPriorityComputer() const;\n+\n+ /** Run internal consistency check (test only). */\n+ void SanityCheck() const;\n+\n+ /** Run a time-dependent consistency check (can only be called immediately after GetRequestable; test only). */\n+ void TimeSanityCheck(std::chrono::microseconds now) const;","path":"src/txrequest.h","commit_id":"fd9a0060f028a4c01bd88f58777dea34bdcbafd1","original_commit_id":"c50b25c2c4c63e1cfea6428913f35926282f3fd8","user":{"login":"sipa","id":548488,"node_id":"MDQ6VXNlcjU0ODQ4OA==","avatar_url":"https://avatars.githubusercontent.com/u/548488?v=4","gravatar_id":"","url":"https://api.github.com/users/sipa","html_url":"https://github.com/sipa","followers_url":"https://api.github.com/users/sipa/followers","following_url":"https://api.github.com/users/sipa/following{/other_user}","gists_url":"https://api.github.com/users/sipa/gists{/gist_id}","starred_url":"https://api.github.com/users/sipa/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sipa/subscriptions","organizations_url":"https://api.github.com/users/sipa/orgs","repos_url":"https://api.github.com/users/sipa/repos","events_url":"https://api.github.com/users/sipa/events{/privacy}","received_events_url":"https://api.github.com/users/sipa/received_events","type":"User","user_view_type":"public","site_admin":false},"body":"Renamed to PostGetRequestableSanityCheck, and updated comment.\r\n\r\nIt's not just a consistency check of request times.","created_at":"2020-09-24T00:23:02Z","updated_at":"2020-10-12T22:08:02Z","html_url":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493970217","pull_request_url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988","_links":{"self":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493970217"},"html":{"href":"https://github.com/bitcoin/bitcoin/pull/19988#discussion_r493970217"},"pull_request":{"href":"https://api.github.com/repos/bitcoin/bitcoin/pulls/19988"}},"reactions":{"url":"https://api.github.com/repos/bitcoin/bitcoin/pulls/comments/493970217/reactions","total_count":0,"+1":0,"-1":0,"laugh":0,"hooray":0,"confused":0,"heart":0,"rocket":0,"eyes":0},"start_line":null,"original_start_line":null,"start_side":null,"line":null,"original_line":223,"side":"RIGHT","in_reply_to_id":493179243,"author_association":"MEMBER","original_position":9,"position":1,"subject_type":"line"}]