Skip to content

Commit

Permalink
Merge #20162: p2p: declare Announcement::m_state as uint8_t, add gett…
Browse files Browse the repository at this point in the history
…er/setter

c8abbc9 p2p: declare Announcement::m_state as uint8_t, add getter/setter (Jon Atack)

Pull request description:

  Change `Announcement::m_state` in `tx_request.cpp` from type `State` to `uint8_t` and add a getter and setter for the conversion to/from `State`. This should silence these travis ci gcc compiler warnings:

  ```
  txrequest.cpp:73:21: warning: ‘{anonymous}::Announcement::m_state’ is
  too small to hold all values of ‘enum class {anonymous}::State’
       State m_state : 3;
                       ^
  ```

  The gcc warnings are based on the maximum value held by the underlying uint8_t enumerator type, even though the intention of the bitfield declaration is the maximum declared enumerator value. They have apparently been silenced in gcc 8.4+ and 9.3+ according to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414.

ACKs for top commit:
  sipa:
    utACK c8abbc9
  ajtowns:
    ACK c8abbc9 -- quick code review
  hebasto:
    ACK c8abbc9, tested on Bionic (x86_64, gcc 7.5.0):

Tree-SHA512: 026721dd7a78983a72da77638d3327d2b252bef804e489278a852f000046c028d6557bbd6c2b4cea391d4e01f9264a1be842d502047cb90b2997cc37bee59e61
  • Loading branch information
MarcoFalke committed Oct 19, 2020
2 parents 4f80734 + c8abbc9 commit 4538501
Showing 1 changed file with 54 additions and 46 deletions.
100 changes: 54 additions & 46 deletions src/txrequest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,32 +69,40 @@ struct Announcement {
/** Whether this is a wtxid request. */
const bool m_is_wtxid : 1;

/** What state this announcement is in. */
State m_state : 3;
/** What state this announcement is in.
* This is a uint8_t instead of a State to silence a GCC warning in versions prior to 8.4 and 9.3.
* See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61414 */
uint8_t m_state : 3;

/** Convert m_state to a State enum. */
State GetState() const { return static_cast<State>(m_state); }

/** Convert a State enum to a uint8_t and store it in m_state. */
void SetState(State state) { m_state = static_cast<uint8_t>(state); }

/** Whether this announcement is selected. There can be at most 1 selected peer per txhash. */
bool IsSelected() const
{
return m_state == State::CANDIDATE_BEST || m_state == State::REQUESTED;
return GetState() == State::CANDIDATE_BEST || GetState() == State::REQUESTED;
}

/** Whether this announcement is waiting for a certain time to pass. */
bool IsWaiting() const
{
return m_state == State::REQUESTED || m_state == State::CANDIDATE_DELAYED;
return GetState() == State::REQUESTED || GetState() == State::CANDIDATE_DELAYED;
}

/** Whether this announcement can feasibly be selected if the current IsSelected() one disappears. */
bool IsSelectable() const
{
return m_state == State::CANDIDATE_READY || m_state == State::CANDIDATE_BEST;
return GetState() == State::CANDIDATE_READY || GetState() == State::CANDIDATE_BEST;
}

/** Construct a new announcement from scratch, initially in CANDIDATE_DELAYED state. */
Announcement(const GenTxid& gtxid, NodeId peer, bool preferred, std::chrono::microseconds reqtime,
SequenceNumber sequence) :
m_txhash(gtxid.GetHash()), m_time(reqtime), m_peer(peer), m_sequence(sequence), m_preferred(preferred),
m_is_wtxid(gtxid.IsWtxid()), m_state(State::CANDIDATE_DELAYED) {}
m_is_wtxid(gtxid.IsWtxid()), m_state(static_cast<uint8_t>(State::CANDIDATE_DELAYED)) {}
};

//! Type alias for priorities.
Expand Down Expand Up @@ -143,7 +151,7 @@ struct ByPeerViewExtractor
using result_type = ByPeerView;
result_type operator()(const Announcement& ann) const
{
return ByPeerView{ann.m_peer, ann.m_state == State::CANDIDATE_BEST, ann.m_txhash};
return ByPeerView{ann.m_peer, ann.GetState() == State::CANDIDATE_BEST, ann.m_txhash};
}
};

Expand All @@ -166,8 +174,8 @@ class ByTxHashViewExtractor {
using result_type = ByTxHashView;
result_type operator()(const Announcement& ann) const
{
const Priority prio = (ann.m_state == State::CANDIDATE_READY) ? m_computer(ann) : 0;
return ByTxHashView{ann.m_txhash, ann.m_state, prio};
const Priority prio = (ann.GetState() == State::CANDIDATE_READY) ? m_computer(ann) : 0;
return ByTxHashView{ann.m_txhash, ann.GetState(), prio};
}
};

Expand Down Expand Up @@ -261,8 +269,8 @@ std::unordered_map<NodeId, PeerInfo> RecomputePeerInfo(const Index& index)
for (const Announcement& ann : index) {
PeerInfo& info = ret[ann.m_peer];
++info.m_total;
info.m_requested += (ann.m_state == State::REQUESTED);
info.m_completed += (ann.m_state == State::COMPLETED);
info.m_requested += (ann.GetState() == State::REQUESTED);
info.m_completed += (ann.GetState() == State::COMPLETED);
}
return ret;
}
Expand All @@ -274,15 +282,15 @@ std::map<uint256, TxHashInfo> ComputeTxHashInfo(const Index& index, const Priori
for (const Announcement& ann : index) {
TxHashInfo& info = ret[ann.m_txhash];
// Classify how many announcements of each state we have for this txhash.
info.m_candidate_delayed += (ann.m_state == State::CANDIDATE_DELAYED);
info.m_candidate_ready += (ann.m_state == State::CANDIDATE_READY);
info.m_candidate_best += (ann.m_state == State::CANDIDATE_BEST);
info.m_requested += (ann.m_state == State::REQUESTED);
info.m_candidate_delayed += (ann.GetState() == State::CANDIDATE_DELAYED);
info.m_candidate_ready += (ann.GetState() == State::CANDIDATE_READY);
info.m_candidate_best += (ann.GetState() == State::CANDIDATE_BEST);
info.m_requested += (ann.GetState() == State::REQUESTED);
// And track the priority of the best CANDIDATE_READY/CANDIDATE_BEST announcements.
if (ann.m_state == State::CANDIDATE_BEST) {
if (ann.GetState() == State::CANDIDATE_BEST) {
info.m_priority_candidate_best = computer(ann);
}
if (ann.m_state == State::CANDIDATE_READY) {
if (ann.GetState() == State::CANDIDATE_READY) {
info.m_priority_best_candidate_ready = std::max(info.m_priority_best_candidate_ready, computer(ann));
}
// Also keep track of which peers this txhash has an announcement for (so we can detect duplicates).
Expand Down Expand Up @@ -369,8 +377,8 @@ class TxRequestTracker::Impl {
Iter<Tag> Erase(Iter<Tag> it)
{
auto peerit = m_peerinfo.find(it->m_peer);
peerit->second.m_completed -= it->m_state == State::COMPLETED;
peerit->second.m_requested -= it->m_state == State::REQUESTED;
peerit->second.m_completed -= it->GetState() == State::COMPLETED;
peerit->second.m_requested -= it->GetState() == State::REQUESTED;
if (--peerit->second.m_total == 0) m_peerinfo.erase(peerit);
return m_index.get<Tag>().erase(it);
}
Expand All @@ -380,11 +388,11 @@ class TxRequestTracker::Impl {
void Modify(Iter<Tag> it, Modifier modifier)
{
auto peerit = m_peerinfo.find(it->m_peer);
peerit->second.m_completed -= it->m_state == State::COMPLETED;
peerit->second.m_requested -= it->m_state == State::REQUESTED;
peerit->second.m_completed -= it->GetState() == State::COMPLETED;
peerit->second.m_requested -= it->GetState() == State::REQUESTED;
m_index.get<Tag>().modify(it, std::move(modifier));
peerit->second.m_completed += it->m_state == State::COMPLETED;
peerit->second.m_requested += it->m_state == State::REQUESTED;
peerit->second.m_completed += it->GetState() == State::COMPLETED;
peerit->second.m_requested += it->GetState() == State::REQUESTED;
}

//! Convert a CANDIDATE_DELAYED announcement into a CANDIDATE_READY. If this makes it the new best
Expand All @@ -393,26 +401,26 @@ class TxRequestTracker::Impl {
void PromoteCandidateReady(Iter<ByTxHash> it)
{
assert(it != m_index.get<ByTxHash>().end());
assert(it->m_state == State::CANDIDATE_DELAYED);
assert(it->GetState() == State::CANDIDATE_DELAYED);
// Convert CANDIDATE_DELAYED to CANDIDATE_READY first.
Modify<ByTxHash>(it, [](Announcement& ann){ ann.m_state = State::CANDIDATE_READY; });
Modify<ByTxHash>(it, [](Announcement& ann){ ann.SetState(State::CANDIDATE_READY); });
// The following code relies on the fact that the ByTxHash is sorted by txhash, and then by state (first
// _DELAYED, then _READY, then _BEST/REQUESTED). Within the _READY announcements, the best one (highest
// priority) comes last. Thus, if an existing _BEST exists for the same txhash that this announcement may
// be preferred over, it must immediately follow the newly created _READY.
auto it_next = std::next(it);
if (it_next == m_index.get<ByTxHash>().end() || it_next->m_txhash != it->m_txhash ||
it_next->m_state == State::COMPLETED) {
it_next->GetState() == State::COMPLETED) {
// This is the new best CANDIDATE_READY, and there is no IsSelected() announcement for this txhash
// already.
Modify<ByTxHash>(it, [](Announcement& ann){ ann.m_state = State::CANDIDATE_BEST; });
} else if (it_next->m_state == State::CANDIDATE_BEST) {
Modify<ByTxHash>(it, [](Announcement& ann){ ann.SetState(State::CANDIDATE_BEST); });
} else if (it_next->GetState() == State::CANDIDATE_BEST) {
Priority priority_old = m_computer(*it_next);
Priority priority_new = m_computer(*it);
if (priority_new > priority_old) {
// There is a CANDIDATE_BEST announcement already, but this one is better.
Modify<ByTxHash>(it_next, [](Announcement& ann){ ann.m_state = State::CANDIDATE_READY; });
Modify<ByTxHash>(it, [](Announcement& ann){ ann.m_state = State::CANDIDATE_BEST; });
Modify<ByTxHash>(it_next, [](Announcement& ann){ ann.SetState(State::CANDIDATE_READY); });
Modify<ByTxHash>(it, [](Announcement& ann){ ann.SetState(State::CANDIDATE_BEST); });
}
}
}
Expand All @@ -427,27 +435,27 @@ class TxRequestTracker::Impl {
auto it_prev = std::prev(it);
// The next best CANDIDATE_READY, if any, immediately precedes the REQUESTED or CANDIDATE_BEST
// announcement in the ByTxHash index.
if (it_prev->m_txhash == it->m_txhash && it_prev->m_state == State::CANDIDATE_READY) {
if (it_prev->m_txhash == it->m_txhash && it_prev->GetState() == State::CANDIDATE_READY) {
// If one such CANDIDATE_READY exists (for this txhash), convert it to CANDIDATE_BEST.
Modify<ByTxHash>(it_prev, [](Announcement& ann){ ann.m_state = State::CANDIDATE_BEST; });
Modify<ByTxHash>(it_prev, [](Announcement& ann){ ann.SetState(State::CANDIDATE_BEST); });
}
}
Modify<ByTxHash>(it, [new_state](Announcement& ann){ ann.m_state = new_state; });
Modify<ByTxHash>(it, [new_state](Announcement& ann){ ann.SetState(new_state); });
}

//! Check if 'it' is the only announcement for a given txhash that isn't COMPLETED.
bool IsOnlyNonCompleted(Iter<ByTxHash> it)
{
assert(it != m_index.get<ByTxHash>().end());
assert(it->m_state != State::COMPLETED); // Not allowed to call this on COMPLETED announcements.
assert(it->GetState() != State::COMPLETED); // Not allowed to call this on COMPLETED announcements.

// This announcement has a predecessor that belongs to the same txhash. Due to ordering, and the
// fact that 'it' is not COMPLETED, its predecessor cannot be COMPLETED here.
if (it != m_index.get<ByTxHash>().begin() && std::prev(it)->m_txhash == it->m_txhash) return false;

// This announcement has a successor that belongs to the same txhash, and is not COMPLETED.
if (std::next(it) != m_index.get<ByTxHash>().end() && std::next(it)->m_txhash == it->m_txhash &&
std::next(it)->m_state != State::COMPLETED) return false;
std::next(it)->GetState() != State::COMPLETED) return false;

return true;
}
Expand All @@ -460,7 +468,7 @@ class TxRequestTracker::Impl {
assert(it != m_index.get<ByTxHash>().end());

// Nothing to be done if it's already COMPLETED.
if (it->m_state == State::COMPLETED) return true;
if (it->GetState() == State::COMPLETED) return true;

if (IsOnlyNonCompleted(it)) {
// This is the last non-COMPLETED announcement for this txhash. Delete all.
Expand Down Expand Up @@ -490,9 +498,9 @@ class TxRequestTracker::Impl {
// and convert them to CANDIDATE_READY and COMPLETED respectively.
while (!m_index.empty()) {
auto it = m_index.get<ByTime>().begin();
if (it->m_state == State::CANDIDATE_DELAYED && it->m_time <= now) {
if (it->GetState() == State::CANDIDATE_DELAYED && it->m_time <= now) {
PromoteCandidateReady(m_index.project<ByTxHash>(it));
} else if (it->m_state == State::REQUESTED && it->m_time <= now) {
} else if (it->GetState() == State::REQUESTED && it->m_time <= now) {
if (expired) expired->emplace_back(it->m_peer, ToGenTxid(*it));
MakeCompleted(m_index.project<ByTxHash>(it));
} else {
Expand Down Expand Up @@ -596,7 +604,7 @@ class TxRequestTracker::Impl {
std::vector<const Announcement*> selected;
auto it_peer = m_index.get<ByPeer>().lower_bound(ByPeerView{peer, true, uint256::ZERO});
while (it_peer != m_index.get<ByPeer>().end() && it_peer->m_peer == peer &&
it_peer->m_state == State::CANDIDATE_BEST) {
it_peer->GetState() == State::CANDIDATE_BEST) {
selected.emplace_back(&*it_peer);
++it_peer;
}
Expand Down Expand Up @@ -625,8 +633,8 @@ class TxRequestTracker::Impl {
// returned by GetRequestable always correspond to CANDIDATE_BEST announcements).

it = m_index.get<ByPeer>().find(ByPeerView{peer, false, txhash});
if (it == m_index.get<ByPeer>().end() || (it->m_state != State::CANDIDATE_DELAYED &&
it->m_state != State::CANDIDATE_READY)) {
if (it == m_index.get<ByPeer>().end() || (it->GetState() != State::CANDIDATE_DELAYED &&
it->GetState() != State::CANDIDATE_READY)) {
// There is no CANDIDATE announcement tracked for this peer, so we have nothing to do. Either this
// txhash wasn't tracked at all (and the caller should have called ReceivedInv), or it was already
// requested and/or completed for other reasons and this is just a superfluous RequestedTx call.
Expand All @@ -638,24 +646,24 @@ class TxRequestTracker::Impl {
// other CANDIDATE_BEST or REQUESTED can exist.
auto it_old = m_index.get<ByTxHash>().lower_bound(ByTxHashView{txhash, State::CANDIDATE_BEST, 0});
if (it_old != m_index.get<ByTxHash>().end() && it_old->m_txhash == txhash) {
if (it_old->m_state == State::CANDIDATE_BEST) {
if (it_old->GetState() == State::CANDIDATE_BEST) {
// The data structure's invariants require that there can be at most one CANDIDATE_BEST or one
// REQUESTED announcement per txhash (but not both simultaneously), so we have to convert any
// existing CANDIDATE_BEST to another CANDIDATE_* when constructing another REQUESTED.
// It doesn't matter whether we pick CANDIDATE_READY or _DELAYED here, as SetTimePoint()
// will correct it at GetRequestable() time. If time only goes forward, it will always be
// _READY, so pick that to avoid extra work in SetTimePoint().
Modify<ByTxHash>(it_old, [](Announcement& ann) { ann.m_state = State::CANDIDATE_READY; });
} else if (it_old->m_state == State::REQUESTED) {
Modify<ByTxHash>(it_old, [](Announcement& ann) { ann.SetState(State::CANDIDATE_READY); });
} else if (it_old->GetState() == State::REQUESTED) {
// As we're no longer waiting for a response to the previous REQUESTED announcement, convert it
// to COMPLETED. This also helps guaranteeing progress.
Modify<ByTxHash>(it_old, [](Announcement& ann) { ann.m_state = State::COMPLETED; });
Modify<ByTxHash>(it_old, [](Announcement& ann) { ann.SetState(State::COMPLETED); });
}
}
}

Modify<ByPeer>(it, [expiry](Announcement& ann) {
ann.m_state = State::REQUESTED;
ann.SetState(State::REQUESTED);
ann.m_time = expiry;
});
}
Expand Down

0 comments on commit 4538501

Please sign in to comment.