Backport PR14897, PR15834 and PR16196
ClosedPublic
Actions

Authored by deadalnix on Nov 28 2019, 14:05.

Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rSTAGING8d1a1da709dc: Backport PR14897, PR15834 and PR16196
rABC8d1a1da709dc: Backport PR14897, PR15834 and PR16196

Summary

They all are backported at once to avoid leaving master in a buggy state.

This is Core PR14897: https://github.com/bitcoin/bitcoin/pull/14897

Change in transaction pull scheduling to prevent InvBlock-related attacks

Co-authored-by: Suhas Daftuar <sdaftuar@gmail.com>

This is Core PR15834: https://github.com/bitcoin/bitcoin/pull/15834

Remove NOTFOUND transactions from in-flight data structures

This prevents a bug where the in-flight queue for our peers will not be
drained, resulting in not downloading any new transactions from our peers.

Thanks to ajtowns for reporting this bug.

Add an explicit memory bound to m_tx_process_time

Previously there was an implicit bound based on the handling of m_tx_announced,
but that approach is error-prone (particularly if we start automatically
removing things from that set).

Improve NOTFOUND comment
Expire old entries from the in-flight tx map

If a peer hasn't responded to a getdata request, eventually time out the request
and remove it from the in-flight data structures. This is to prevent any bugs in
our handling of those in-flight data structures from filling up the in-flight
map and preventing us from requesting more transactions (such as the NOTFOUND
bug, fixed in a previous commit).

Co-authored-by: Anthony Towns <aj@erisian.com.au>

Fix bug around transaction requests

If a transaction is already in-flight when a peer announces a new tx to us, we
schedule a time in the future to reconsider whether to download. At that future
time, there was a bug that would prevent transactions from being rescheduled
for potential download again (ie if the transaction was still in-flight at the
time of reconsideration, such as from some other peer). Fix this.

This is Core PR16196: https://github.com/bitcoin/bitcoin/pull/16196

doc: Add release notes for 14897 & 15834

Test Plan

make check
./test/functional/test_runner.py --extended

Diff Detail

Repository

rABC Bitcoin ABC

Branch

randomizeandfixbug

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 8324
Build 14667: Default Diff Build & Tests
Build 14666: arc lint + arc unit

Event Timeline

deadalnix created this revision.Nov 28 2019, 14:05

Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 28 2019, 14:05

Build Bitcoin-ABC / Diffs / Diff Testing started.

deadalnix mentioned this in D4513: Change in transaction pull scheduling to prevent InvBlock-related attacks.Nov 28 2019, 14:06

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B8323: Diff 14485.Nov 28 2019, 14:23

Fabien requested changes to this revision.Nov 28 2019, 15:28

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/net_processing.cpp
382 ↗	(On Diff #14485)	`TxId`
385 ↗	(On Diff #14485)	Dito
2436 ↗	(On Diff #14485)	Nit: add a newline to avoid later merge conflict
2753 ↗	(On Diff #14485)	Remove
3594 ↗	(On Diff #14485)	Nit: in_flight_it can be const. Also maybe make inv.hash a `const TxId` ? const TxId txid(inv.hash); auto in_flight_it = state->m_tx_download.m_tx_in_flight.find(txid); [...] state->m_tx_download.m_tx_announced.erase(txid);
4645 ↗	(On Diff #14485)	`inv.hash` => `txid`
4657 ↗	(On Diff #14485)	Dito
4658 ↗	(On Diff #14485)	Dito