Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select orphan transaction uniformly for eviction #14626

Merged
merged 1 commit into from Feb 14, 2019

Conversation

@sipa
Copy link
Member

@sipa sipa commented Nov 1, 2018

The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.

@fanquake fanquake added the P2P label Nov 1, 2018
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Nov 1, 2018

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

Conflicts

No conflicts as of last run.

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Nov 3, 2018

Concept ACK.

src/net_processing.cpp Outdated Show resolved Hide resolved
src/net_processing.cpp Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
The previous code was biased towards evicting transactions whose txid has
a larger gap (lexicographically) with the previous txid in the orphan pool.
@sipa sipa force-pushed the 201810_uniform_orphan_eviction branch from 64e1dbb to 7257353 Dec 13, 2018
@sipa
Copy link
Member Author

@sipa sipa commented Dec 13, 2018

Rebased.

@@ -170,6 +171,8 @@ namespace {
};
std::map<COutPoint, std::set<std::map<uint256, COrphanTx>::iterator, IteratorComparator>> mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans);

std::vector<std::map<uint256, COrphanTx>::iterator> g_orphan_list GUARDED_BY(g_cs_orphans); //! For random eviction
Copy link
Member

@Empact Empact Jan 31, 2019

//!< for in-line doxygen

Copy link
Member Author

@sipa sipa Feb 3, 2019

It's not storing a COrphanTx inside. It's storing a list of iterators to entries in a map from uint256 to COrphanTx.

Such iterators are only as large as one pointer.

@Empact
Copy link
Member

@Empact Empact commented Jan 31, 2019

utACK 7257353

@sipa
Copy link
Member Author

@sipa sipa commented Feb 2, 2019

@sdaftuar @naumenkogs Feel like reviewing this?

Copy link
Member

@naumenkogs naumenkogs left a comment

Concept ACK, will look closer.

First of all, we either should remove this comment or do what it says?
struct COrphanTx { // When modifying, adapt the copy of this definition in tests/DoS_tests.

@@ -170,6 +171,8 @@ namespace {
};
std::map<COutPoint, std::set<std::map<uint256, COrphanTx>::iterator, IteratorComparator>> mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans);

std::vector<std::map<uint256, COrphanTx>::iterator> g_orphan_list GUARDED_BY(g_cs_orphans); //! For random eviction
Copy link
Member

@naumenkogs naumenkogs Feb 3, 2019

Is there any benefit of having COrphanTx inside, why not just using a hash?

Copy link
Member

@naumenkogs naumenkogs Feb 3, 2019

Right, I guess it's cleaner this way. Feel free to resolve this one.

@sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Feb 4, 2019

utACK 7257353

@gmaxwell
Copy link
Contributor

@gmaxwell gmaxwell commented Feb 14, 2019

ACK

@MarcoFalke MarcoFalke merged commit 7257353 into bitcoin:master Feb 14, 2019
2 checks passed
MarcoFalke added a commit that referenced this issue Feb 14, 2019
7257353 Select orphan transaction uniformly for eviction (Pieter Wuille)

Pull request description:

  The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.

Tree-SHA512: e35f700aea5ed79d1bc57f64bffcb623424b40156fd0a12f05f74f981a8aa4175d5c18d042989243f7559242bdf1d6d720bcf588d28f43d74a798a4843f09c70
// Unless we're deleting the last entry in g_orphan_list, move the last
// entry to the position we're deleting.
auto it_last = g_orphan_list.back();
g_orphan_list[old_pos] = it_last;
Copy link
Member

@MarcoFalke MarcoFalke Feb 14, 2019

style-nit: Looks odd to assign an iterator. (The code is still correct, since back() returns a reference, but the naming might be wrong)

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Feb 14, 2019

ACK

deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jun 10, 2020
Summary:
7257353b93 Select orphan transaction uniformly for eviction (Pieter Wuille)

Pull request description:

  The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.

Tree-SHA512: e35f700aea5ed79d1bc57f64bffcb623424b40156fd0a12f05f74f981a8aa4175d5c18d042989243f7559242bdf1d6d720bcf588d28f43d74a798a4843f09c70

Backport of Core [[bitcoin/bitcoin#14626 | PR14626]]

Test Plan:
  ninja
  ninja check-all

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D6488
size_t randompos = rng.randrange(g_orphan_list.size());
EraseOrphanTx(g_orphan_list[randompos]->first);
Copy link
Member

@hebasto hebasto Jun 24, 2020

@sipa

As the mapOrphanTransactions items are sorted by hash, the first item is random with a negligible bias:

Suggested change
size_t randompos = rng.randrange(g_orphan_list.size());
EraseOrphanTx(g_orphan_list[randompos]->first);
EraseOrphanTx(mapOrphanTransactions.begin()->first);

Or did I miss something?

Copy link
Member Author

@sipa sipa Jun 24, 2020

That would always evict lower txids before higher txids. Not exactly negligible...

Copy link
Member

@hebasto hebasto Jun 24, 2020

That would always evict lower txids before higher txids. Not exactly negligible...

I do not mean "random txid", rather "random transaction".

Copy link
Member Author

@sipa sipa Jun 24, 2020

No. That would apply if the transactions were secret, but since an attacker may know the transactions, they also know the txids. The hashing applied is not relevant.

If mapOrphanTransactions was say an unordered_map, and was using salted hash for its internal hashing (to convert txids to bucket positions) like is used in some places, then this would perhaps not be an issue.

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 18, 2021
7257353 Select orphan transaction uniformly for eviction (Pieter Wuille)

Pull request description:

  The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.

Tree-SHA512: e35f700aea5ed79d1bc57f64bffcb623424b40156fd0a12f05f74f981a8aa4175d5c18d042989243f7559242bdf1d6d720bcf588d28f43d74a798a4843f09c70
Signed-off-by: pasta <pasta@dashboost.org>

# Conflicts:
#	src/net_processing.cpp
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 18, 2021
7257353 Select orphan transaction uniformly for eviction (Pieter Wuille)

Pull request description:

  The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.

Tree-SHA512: e35f700aea5ed79d1bc57f64bffcb623424b40156fd0a12f05f74f981a8aa4175d5c18d042989243f7559242bdf1d6d720bcf588d28f43d74a798a4843f09c70
Signed-off-by: pasta <pasta@dashboost.org>

# Conflicts:
#	src/net_processing.cpp
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 19, 2021
7257353 Select orphan transaction uniformly for eviction (Pieter Wuille)

Pull request description:

  The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.

Tree-SHA512: e35f700aea5ed79d1bc57f64bffcb623424b40156fd0a12f05f74f981a8aa4175d5c18d042989243f7559242bdf1d6d720bcf588d28f43d74a798a4843f09c70
Signed-off-by: pasta <pasta@dashboost.org>

# Conflicts:
#	src/net_processing.cpp
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 19, 2021
7257353 Select orphan transaction uniformly for eviction (Pieter Wuille)

Pull request description:

  The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.

Tree-SHA512: e35f700aea5ed79d1bc57f64bffcb623424b40156fd0a12f05f74f981a8aa4175d5c18d042989243f7559242bdf1d6d720bcf588d28f43d74a798a4843f09c70
Signed-off-by: pasta <pasta@dashboost.org>

# Conflicts:
#	src/net_processing.cpp
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 20, 2021
7257353 Select orphan transaction uniformly for eviction (Pieter Wuille)

Pull request description:

  The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.

Tree-SHA512: e35f700aea5ed79d1bc57f64bffcb623424b40156fd0a12f05f74f981a8aa4175d5c18d042989243f7559242bdf1d6d720bcf588d28f43d74a798a4843f09c70
Signed-off-by: pasta <pasta@dashboost.org>
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 20, 2021
7257353 Select orphan transaction uniformly for eviction (Pieter Wuille)

Pull request description:

  The previous code was biased towards evicting transactions whose txid has a larger gap (lexicographically) with the previous txid in the orphan pool.

Tree-SHA512: e35f700aea5ed79d1bc57f64bffcb623424b40156fd0a12f05f74f981a8aa4175d5c18d042989243f7559242bdf1d6d720bcf588d28f43d74a798a4843f09c70
Signed-off-by: pasta <pasta@dashboost.org>

# Conflicts:
#	src/net_processing.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet