Skip to content

Commit 4f73f04

Browse files
l0rincPiRK
l0rinc
authored and
PiRK
committed
coins, refactor: Remove direct GetFlags access
Summary: We don't need so much access to the internals of CCoinsCacheEntry, since many tests are just exercising invalid combinations this way. This implies that `AddFlags` has private access now. This is a partial backport of [[bitcoin/bitcoin#30906 | core#30906]] bitcoin/bitcoin@15aaa81 Depends on D18808 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D18809
1 parent 1b65cdc commit 4f73f04

File tree

3 files changed

+47
-43
lines changed

3 files changed

+47
-43
lines changed

‎src/coins.h‎

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,22 @@ struct CCoinsCacheEntry {
114114
CoinsCachePair *m_next{nullptr};
115115
uint8_t m_flags{0};
116116

117+
//! Adding a flag requires a reference to the sentinel of the flagged pair
118+
//! linked list.
119+
static void AddFlags(uint8_t flags, CoinsCachePair &pair,
120+
CoinsCachePair &sentinel) noexcept {
121+
Assume(flags & (DIRTY | FRESH));
122+
if (!pair.second.m_flags) {
123+
Assume(!pair.second.m_prev && !pair.second.m_next);
124+
pair.second.m_prev = sentinel.second.m_prev;
125+
pair.second.m_next = &sentinel;
126+
sentinel.second.m_prev = &pair;
127+
pair.second.m_prev->second.m_next = &pair;
128+
}
129+
Assume(pair.second.m_prev && pair.second.m_next);
130+
pair.second.m_flags |= flags;
131+
}
132+
117133
public:
118134
// The actual cached data.
119135
Coin coin;
@@ -143,22 +159,6 @@ struct CCoinsCacheEntry {
143159
explicit CCoinsCacheEntry(Coin &&coin_) : coin(std::move(coin_)) {}
144160
~CCoinsCacheEntry() { SetClean(); }
145161

146-
//! Adding a flag also requires a self reference to the pair that contains
147-
//! this entry in the CCoinsCache map and a reference to the sentinel of the
148-
//! flagged pair linked list.
149-
static void AddFlags(uint8_t flags, CoinsCachePair &pair,
150-
CoinsCachePair &sentinel) noexcept {
151-
Assume(flags & (DIRTY | FRESH));
152-
if (!pair.second.m_flags) {
153-
Assume(!pair.second.m_prev && !pair.second.m_next);
154-
pair.second.m_prev = sentinel.second.m_prev;
155-
pair.second.m_next = &sentinel;
156-
sentinel.second.m_prev = &pair;
157-
pair.second.m_prev->second.m_next = &pair;
158-
}
159-
Assume(pair.second.m_prev && pair.second.m_next);
160-
pair.second.m_flags |= flags;
161-
}
162162
static void SetDirty(CoinsCachePair &pair,
163163
CoinsCachePair &sentinel) noexcept {
164164
AddFlags(DIRTY, pair, sentinel);
@@ -177,7 +177,6 @@ struct CCoinsCacheEntry {
177177
m_flags = 0;
178178
m_prev = m_next = nullptr;
179179
}
180-
uint8_t GetFlags() const noexcept { return m_flags; }
181180
bool IsDirty() const noexcept { return m_flags & DIRTY; }
182181
bool IsFresh() const noexcept { return m_flags & FRESH; }
183182

‎src/test/coins_tests.cpp‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -662,7 +662,13 @@ void GetCoinMapEntry(const CCoinsMap &map, Amount &value, char &flags,
662662
} else {
663663
value = it->second.coin.GetTxOut().nValue;
664664
}
665-
flags = it->second.GetFlags();
665+
flags = 0;
666+
if (it->second.IsDirty()) {
667+
flags |= DIRTY;
668+
}
669+
if (it->second.IsFresh()) {
670+
flags |= FRESH;
671+
}
666672
assert(flags != NO_ENTRY);
667673
}
668674
}

‎src/test/coinscachepair_tests.cpp‎

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ std::list<CoinsCachePair> CreatePairs(CoinsCachePair &sentinel) {
2020
auto node{std::prev(nodes.end())};
2121
CCoinsCacheEntry::SetDirty(*node, sentinel);
2222

23-
BOOST_CHECK_EQUAL(node->second.GetFlags(), CCoinsCacheEntry::DIRTY);
23+
BOOST_CHECK(node->second.IsDirty() && !node->second.IsFresh());
2424
BOOST_CHECK_EQUAL(node->second.Next(), &sentinel);
2525
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*node));
2626

@@ -46,7 +46,7 @@ BOOST_AUTO_TEST_CASE(linked_list_iteration) {
4646
BOOST_CHECK_EQUAL(node, &sentinel);
4747

4848
// Check iterating through pairs is identical to iterating through a list
49-
// Clear the flags during iteration
49+
// Clear the state during iteration
5050
node = sentinel.second.Next();
5151
for (const auto &expected : nodes) {
5252
BOOST_CHECK_EQUAL(&expected, node);
@@ -62,7 +62,7 @@ BOOST_AUTO_TEST_CASE(linked_list_iteration) {
6262
// Delete the nodes from the list to make sure there are no dangling
6363
// pointers
6464
for (auto it{nodes.begin()}; it != nodes.end(); it = nodes.erase(it)) {
65-
BOOST_CHECK_EQUAL(it->second.GetFlags(), 0);
65+
BOOST_CHECK(!it->second.IsDirty() && !it->second.IsFresh());
6666
}
6767
}
6868

@@ -72,8 +72,8 @@ BOOST_AUTO_TEST_CASE(linked_list_iterate_erase) {
7272
auto nodes{CreatePairs(sentinel)};
7373

7474
// Check iterating through pairs is identical to iterating through a list
75-
// Erase the nodes as we iterate through, but don't clear flags
76-
// The flags will be cleared by the CCoinsCacheEntry's destructor
75+
// Erase the nodes as we iterate through, but don't clear state
76+
// The state will be cleared by the CCoinsCacheEntry's destructor
7777
auto node{sentinel.second.Next()};
7878
for (auto expected{nodes.begin()}; expected != nodes.end();
7979
expected = nodes.erase(expected)) {
@@ -102,19 +102,19 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) {
102102
// sentinel->n1->n3->n4->sentinel
103103
nodes.erase(n2);
104104
// Check that n1 now points to n3, and n3 still points to n4
105-
// Also check that flags were not altered
106-
BOOST_CHECK_EQUAL(n1->second.GetFlags(), CCoinsCacheEntry::DIRTY);
105+
// Also check that state was not altered
106+
BOOST_CHECK(n1->second.IsDirty() && !n1->second.IsFresh());
107107
BOOST_CHECK_EQUAL(n1->second.Next(), &(*n3));
108-
BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY);
108+
BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh());
109109
BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4));
110110
BOOST_CHECK_EQUAL(n3->second.Prev(), &(*n1));
111111

112112
// Delete n1
113113
// sentinel->n3->n4->sentinel
114114
nodes.erase(n1);
115115
// Check that sentinel now points to n3, and n3 still points to n4
116-
// Also check that flags were not altered
117-
BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY);
116+
// Also check that state was not altered
117+
BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh());
118118
BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3));
119119
BOOST_CHECK_EQUAL(n3->second.Next(), &(*n4));
120120
BOOST_CHECK_EQUAL(n3->second.Prev(), &sentinel);
@@ -123,8 +123,8 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) {
123123
// sentinel->n3->sentinel
124124
nodes.erase(n4);
125125
// Check that sentinel still points to n3, and n3 points to sentinel
126-
// Also check that flags were not altered
127-
BOOST_CHECK_EQUAL(n3->second.GetFlags(), CCoinsCacheEntry::DIRTY);
126+
// Also check that state was not altered
127+
BOOST_CHECK(n3->second.IsDirty() && !n3->second.IsFresh());
128128
BOOST_CHECK_EQUAL(sentinel.second.Next(), &(*n3));
129129
BOOST_CHECK_EQUAL(n3->second.Next(), &sentinel);
130130
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &(*n3));
@@ -137,56 +137,55 @@ BOOST_AUTO_TEST_CASE(linked_list_random_deletion) {
137137
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &sentinel);
138138
}
139139

140-
BOOST_AUTO_TEST_CASE(linked_list_add_flags) {
140+
BOOST_AUTO_TEST_CASE(linked_list_set_state) {
141141
CoinsCachePair sentinel;
142142
sentinel.second.SelfRef(sentinel);
143143
CoinsCachePair n1;
144144
CoinsCachePair n2;
145145

146-
// Check that adding DIRTY flag inserts it into linked list and sets flags
146+
// Check that setting DIRTY inserts it into linked list and sets state
147147
CCoinsCacheEntry::SetDirty(n1, sentinel);
148-
BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
148+
BOOST_CHECK(n1.second.IsDirty() && !n1.second.IsFresh());
149149
BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel);
150150
BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
151151
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
152152
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n1);
153153

154-
// Check that adding FRESH flag on new node inserts it after n1
154+
// Check that setting FRESH on new node inserts it after n1
155155
CCoinsCacheEntry::SetFresh(n2, sentinel);
156-
BOOST_CHECK_EQUAL(n2.second.GetFlags(), CCoinsCacheEntry::FRESH);
156+
BOOST_CHECK(n2.second.IsFresh() && !n2.second.IsDirty());
157157
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
158158
BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
159159
BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
160160
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
161161

162-
// Check that we can add extra flags, but they don't change our position
162+
// Check that we can set extra state, but they don't change our position
163163
CCoinsCacheEntry::SetFresh(n1, sentinel);
164-
BOOST_CHECK_EQUAL(n1.second.GetFlags(),
165-
CCoinsCacheEntry::DIRTY | CCoinsCacheEntry::FRESH);
164+
BOOST_CHECK(n1.second.IsDirty() && n1.second.IsFresh());
166165
BOOST_CHECK_EQUAL(n1.second.Next(), &n2);
167166
BOOST_CHECK_EQUAL(n1.second.Prev(), &sentinel);
168167
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n1);
169168
BOOST_CHECK_EQUAL(n2.second.Prev(), &n1);
170169

171-
// Check that we can clear flags then re-add them
170+
// Check that we can clear state then re-set it
172171
n1.second.SetClean();
173-
BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
172+
BOOST_CHECK(!n1.second.IsDirty() && !n1.second.IsFresh());
174173
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
175174
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
176175
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
177176
BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
178177

179-
// Check that calling `SetClean` with 0 flags has no effect
178+
// Calling `SetClean` a second time has no effect
180179
n1.second.SetClean();
181-
BOOST_CHECK_EQUAL(n1.second.GetFlags(), 0);
180+
BOOST_CHECK(!n1.second.IsDirty() && !n1.second.IsFresh());
182181
BOOST_CHECK_EQUAL(sentinel.second.Next(), &n2);
183182
BOOST_CHECK_EQUAL(sentinel.second.Prev(), &n2);
184183
BOOST_CHECK_EQUAL(n2.second.Next(), &sentinel);
185184
BOOST_CHECK_EQUAL(n2.second.Prev(), &sentinel);
186185

187186
// Adding DIRTY re-inserts it after n2
188187
CCoinsCacheEntry::SetDirty(n1, sentinel);
189-
BOOST_CHECK_EQUAL(n1.second.GetFlags(), CCoinsCacheEntry::DIRTY);
188+
BOOST_CHECK(n1.second.IsDirty() && !n1.second.IsFresh());
190189
BOOST_CHECK_EQUAL(n2.second.Next(), &n1);
191190
BOOST_CHECK_EQUAL(n1.second.Prev(), &n2);
192191
BOOST_CHECK_EQUAL(n1.second.Next(), &sentinel);

0 commit comments

Comments
 (0)