Transmission/transmission
1
0
Fork 0
mirror of https://github.com/transmission/transmission.git synced 2026-04-17 23:54:19 +01:00
Code Issues Packages Projects Releases Wiki Activity

fix: re-create candidate when got bad block if needed (#8654)

Browse Source 
* refactor: use the same salter everywhere

* fix: re-create candidate when got bad block if needed

* refactor: housekeeping

* fix: emit bad piece signal after all bookkeeping

(cherry picked from commit 59abf6e1db)
This commit is contained in:
Yat Ho
2026-03-06 00:00:43 +08:00
committed by GitHub
parent fe30368323
commit db9a0cc6ec
3 changed files with 116 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

53
libtransmission/peer-mgr-wishlist.cc
View File

@@ -263,14 +263,16 @@ private:
void got_bad_piece(tr_piece_index_t const piece)
{
auto const iter = find_by_piece(piece);
if (iter == std::end(candidates_))
auto iter = find_by_piece(piece);
if (auto const salt = get_salt(piece); iter != std::end(candidates_))
{
return;
*iter = { piece, salt, &mediator_ };
}
else
{
iter = candidates_.emplace(iter, piece, salt, &mediator_);
}
TR_ASSERT(std::empty(iter->unrequested));
iter->block_span = iter->raw_block_span;
if (piece > 0U)
{
if (auto const prev = find_by_piece(piece - 1U); prev != std::end(candidates_))
@@ -279,7 +281,9 @@ private:
TR_ASSERT(iter->block_span.begin == prev->block_span.end);
for (tr_block_index_t i = iter->block_span.begin; i > iter->raw_block_span.begin; --i)
{
prev->unrequested.insert(i - 1U);
auto const block = i - 1U;
prev->unrequested.insert(block);
iter->unrequested.erase(block);
}
}
}
@@ -291,16 +295,13 @@ private:
TR_ASSERT(iter->block_span.end == next->block_span.begin);
for (tr_block_index_t i = iter->raw_block_span.end; i > iter->block_span.end; --i)
{
next->unrequested.insert(i - 1U);
auto const block = i - 1U;
next->unrequested.insert(block);
iter->unrequested.erase(block);
}
}
}
for (auto [begin, i] = iter->block_span; i > begin; --i)
{
iter->unrequested.insert(i - 1U);
}
std::sort(std::begin(candidates_), std::end(candidates_));
}
@@ -322,16 +323,15 @@ private:
[block](auto const& c) { return c.block_belongs(block); });
}
static constexpr tr_piece_index_t get_salt(
tr_piece_index_t const piece,
tr_piece_index_t const n_pieces,
tr_piece_index_t const random_salt,
bool const is_sequential,
tr_piece_index_t const sequential_download_from_piece)
tr_piece_index_t get_salt(tr_piece_index_t const piece)
{
auto const is_sequential = mediator_.is_sequential_download();
auto const sequential_download_from_piece = mediator_.sequential_download_from_piece();
auto const n_pieces = mediator_.piece_count();
if (!is_sequential)
{
return random_salt;
return salter_();
}
// Download first and last piece first
@@ -365,9 +365,6 @@ private:
void candidate_list_upkeep()
{
auto n_old_c = std::size(candidates_);
auto salter = tr_salt_shaker<tr_piece_index_t>{};
auto const is_sequential = mediator_.is_sequential_download();
auto const sequential_download_from_piece = mediator_.sequential_download_from_piece();
auto const n_pieces = mediator_.piece_count();
candidates_.reserve(n_pieces);
@@ -405,7 +402,7 @@ private:
}
else
{
auto const salt = get_salt(piece, n_pieces, salter(), is_sequential, sequential_download_from_piece);
auto const salt = get_salt(piece);
auto& candidate = candidates_.emplace_back(piece, salt, &mediator_);
if (auto& begin = candidate.block_span.begin; prev != nullptr)
@@ -482,13 +479,9 @@ private:
void recalculate_salt()
{
auto salter = tr_salt_shaker<tr_piece_index_t>{};
auto const is_sequential = mediator_.is_sequential_download();
auto const sequential_download_from_piece = mediator_.sequential_download_from_piece();
auto const n_pieces = mediator_.piece_count();
for (auto& candidate : candidates_)
{
candidate.salt = get_salt(candidate.piece, n_pieces, salter(), is_sequential, sequential_download_from_piece);
candidate.salt = get_salt(candidate.piece);
}
std::sort(std::begin(candidates_), std::end(candidates_));
@@ -526,10 +519,14 @@ private:
}
}
// ---
CandidateVec candidates_;
std::array<libtransmission::ObserverTag, 15U> const tags_;
tr_salt_shaker<tr_piece_index_t> salter_ = {};
Mediator& mediator_;
};

2
libtransmission/torrent.cc
View File

@@ -2186,8 +2186,8 @@ void tr_torrent::on_piece_failed(tr_piece_index_t const piece)
auto const n = piece_size(piece);
bytes_corrupt_ += n;
bytes_downloaded_.reduce(n);
got_bad_piece_.emit(this, piece);
set_has_piece(piece, false);
got_bad_piece_.emit(this, piece);
}
void tr_torrent::on_block_received(tr_block_index_t const block)

91
tests/libtransmission/peer-mgr-wishlist-test.cc
View File

@@ -2227,7 +2227,7 @@ TEST_F(PeerMgrWishlistTest, unalignedTorrentGot2ConsectutiveBadPieces)
auto requested = tr_bitfield{ 134 };
auto const spans = get_spans(67);
// We should get 1 pan [33, 100),
// We should get 1 span [33, 100),
// not [33, 67), [66, 100)
EXPECT_EQ(std::size(spans), 1);
@@ -2243,6 +2243,95 @@ TEST_F(PeerMgrWishlistTest, unalignedTorrentGot2ConsectutiveBadPieces)
}
}
TEST_F(PeerMgrWishlistTest, unalignedTorrentPreviouslyCompletedPieceCorrupted)
{
auto const get_spans = [this](size_t n_wanted)
{
auto mediator = MockMediator{ *this };
// setup: 4 pieces, (100 / 3 * 16) KiB each
// N.B. only the boundary of piece 2 and 3 is aligned
mediator.block_span_[0] = { 0, 34 };
mediator.block_span_[1] = { 33, 67 };
mediator.block_span_[2] = { 66, 100 };
mediator.block_span_[3] = { 100, 134 };
// peer has all pieces
mediator.piece_replication_[0] = 1;
mediator.piece_replication_[1] = 1;
mediator.piece_replication_[2] = 1;
mediator.piece_replication_[3] = 1;
// we want all 4 pieces
mediator.client_wants_piece_.insert(0);
mediator.client_wants_piece_.insert(1);
mediator.client_wants_piece_.insert(2);
mediator.client_wants_piece_.insert(3);
// allow the wishlist to build its cache
auto wishlist = Wishlist{ mediator };
// pieces 0, 2, 3 completed normally
sent_request_.emit(nullptr, nullptr, { 0, 134 });
for (tr_block_index_t block = 0; block < 134; ++block)
{
mediator.client_has_block_.insert(block);
got_block_.emit(nullptr, block);
}
mediator.client_has_piece_.insert(0);
piece_completed_.emit(nullptr, 0);
mediator.client_has_piece_.insert(3);
piece_completed_.emit(nullptr, 3);
mediator.client_has_piece_.insert(2);
piece_completed_.emit(nullptr, 2);
// piece 1 turned out to be corrupt, needs to be re-downloaded
for (auto [block, end] = mediator.block_span_[1]; block < end; ++block)
{
mediator.client_has_block_.erase(block);
}
got_bad_piece_.emit(nullptr, 1);
// receive the boundary block 66
mediator.client_has_block_.insert(66);
got_block_.emit(nullptr, 66);
// piece 2 fails the re-check and needs to be re-downloaded
for (auto [block, end] = mediator.block_span_[2]; block < end; ++block)
{
mediator.client_has_block_.erase(block);
}
mediator.client_has_piece_.erase(2);
got_bad_piece_.emit(nullptr, 2);
return wishlist.next(n_wanted, PeerHasAllPieces);
};
// NB: when all other things are equal in the wishlist, pieces are
// picked at random so this test -could- pass even if there's a bug.
// So test several times to shake out any randomness
static auto constexpr NumRuns = 1000;
for (int run = 0; run < NumRuns; ++run)
{
auto requested = tr_bitfield{ 134 };
auto const spans = get_spans(67);
// We should get 1 span [33, 100),
// not [33, 67), [66, 100), or any one of them
EXPECT_EQ(std::size(spans), 1);
// Since the spans might overlap if we didn't handle unaligned
// torrents correctly, we might not get all 67 blocks if there
// is a bug
for (auto const& [begin, end] : spans)
{
requested.set_span(begin, end);
}
EXPECT_EQ(67U, requested.count());
EXPECT_EQ(67U, requested.count(33, 100));
}
}
TEST_F(PeerMgrWishlistTest, unalignedTorrentPartiallyWanted)
{
auto const get_spans = [this](size_t n_wanted)