Skip to content

Commit 80a699f

Browse files
author
MarcoFalke
committed
Merge bitcoin#21525: [Bundle 4.5/n] Followup fixups to bundle 4
693414d node/ifaces: ChainImpl: Use an accessor for ChainMan (Carl Dong) 98c4e25 node/ifaces: NodeImpl: Use an accessor for ChainMan (Carl Dong) 7e8b5ee validation: Make BlockManager::LookupBlockIndex const (Carl Dong) 88aead2 node: Avoid potential UB by asserting assumptions (Carl Dong) 1dd8ed7 net_processing: Move comments to declarations (Carl Dong) 07156eb node/coinstats: Replace #include with fwd-declaration (Carl Dong) 7b8e976 miner: Add chainstate member to BlockAssembler (Carl Dong) e62067e Revert "miner: Pass in chainstate to BlockAssembler::CreateNewBlock" (Carl Dong) eede064 Revert "scripted-diff: Invoke CreateNewBlock with chainstate" (Carl Dong) 0c1b2bc Revert "miner: Remove old CreateNewBlock w/o chainstate param" (Carl Dong) Pull request description: Chronological history of this changeset: 1. Bundle 4 (bitcoin#21270) got merged 2. Posthumous reviews were posted 3. These changes were prepended in bundle 5 4. More reviews were added in bundle 5 5. Someone suggested that we split the prepended changes up to another PR 6. This is that PR In the future, I will just do posthumous review changes in another PR instead. I apologize for the confusion. Addresses posthumous reviews on bundle 4: - From jnewbery: - bitcoin#21270 (comment) - I didn't fix this one, but I added a `TODO` comment so that we don't lost track of it - bitcoin#21270 (comment) - bitcoin#21270 (comment) - bitcoin#21270 (comment) - bitcoin#21270 (comment) - From MarcoFalke: - bitcoin#21270 (comment) - bitcoin#21270 (comment) - bitcoin#21270 (comment) - bitcoin#21270 (comment) Addresses reviews on bundle 5: - Checking chainman existence before locking cs_main - MarcoFalke - bitcoin#21391 (comment) - bitcoin#21391 (comment) - Appropriate locking, usage of chainman, and control flow in `src/node/interfaces.cpp` - MarcoFalke - bitcoin#21391 (comment) - jnewbery - bitcoin#21391 (comment) - bitcoin#21391 (comment) - ryanofsky - bitcoin#21391 (comment) - Style/comment formatting changes - jnewbery - bitcoin#21391 (comment) - bitcoin#21391 (comment) - Making LookupBlockIndex const - jnewbery - bitcoin#21391 (comment) ACKs for top commit: MarcoFalke: review ACK 693414d 🛐 ryanofsky: Code review ACK 693414d. I reviewed this previously as part of bitcoin#21391. I am a fan of the increasingly complicated bundle numbering, and kind of hope there in the next round there is some way we can get bundles 5.333333 and 5.666667! jamesob: ACK 693414d ([`jamesob/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f`](https://github.com/jamesob/bitcoin/tree/ackr/21525.1.dongcarl.bundle_4_5_n_followup_f)) Tree-SHA512: 9bdc199f70400d01764e1bd03c25bdb6cff26dcef60e4ca3b649baf8d017a2dfc1f058099067962b4b6ccd32d078002b1389d733039f4c337558cb70324c0ee3
2 parents c2caa0f + 693414d commit 80a699f

15 files changed

+148
-115
lines changed

src/miner.cpp

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,10 @@ BlockAssembler::Options::Options() {
5555
nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT;
5656
}
5757

58-
BlockAssembler::BlockAssembler(const CTxMemPool& mempool, const CChainParams& params, const Options& options)
58+
BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options)
5959
: chainparams(params),
60-
m_mempool(mempool)
60+
m_mempool(mempool),
61+
m_chainstate(chainstate)
6162
{
6263
blockMinFeeRate = options.blockMinFeeRate;
6364
// Limit weight to between 4K and MAX_BLOCK_WEIGHT-4K for sanity:
@@ -79,8 +80,8 @@ static BlockAssembler::Options DefaultOptions()
7980
return options;
8081
}
8182

82-
BlockAssembler::BlockAssembler(const CTxMemPool& mempool, const CChainParams& params)
83-
: BlockAssembler(mempool, params, DefaultOptions()) {}
83+
BlockAssembler::BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params)
84+
: BlockAssembler(chainstate, mempool, params, DefaultOptions()) {}
8485

8586
void BlockAssembler::resetBlock()
8687
{
@@ -96,7 +97,7 @@ void BlockAssembler::resetBlock()
9697
nFees = 0;
9798
}
9899

99-
std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(CChainState& chainstate, const CScript& scriptPubKeyIn)
100+
std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn)
100101
{
101102
int64_t nTimeStart = GetTimeMicros();
102103

@@ -114,8 +115,8 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(CChainState& chai
114115
pblocktemplate->vTxSigOpsCost.push_back(-1); // updated at end
115116

116117
LOCK2(cs_main, m_mempool.cs);
117-
assert(std::addressof(*::ChainActive().Tip()) == std::addressof(*chainstate.m_chain.Tip()));
118-
CBlockIndex* pindexPrev = chainstate.m_chain.Tip();
118+
assert(std::addressof(*::ChainActive().Tip()) == std::addressof(*m_chainstate.m_chain.Tip()));
119+
CBlockIndex* pindexPrev = m_chainstate.m_chain.Tip();
119120
assert(pindexPrev != nullptr);
120121
nHeight = pindexPrev->nHeight + 1;
121122

@@ -174,8 +175,8 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(CChainState& chai
174175
pblocktemplate->vTxSigOpsCost[0] = WITNESS_SCALE_FACTOR * GetLegacySigOpCount(*pblock->vtx[0]);
175176

176177
BlockValidationState state;
177-
assert(std::addressof(::ChainstateActive()) == std::addressof(chainstate));
178-
if (!TestBlockValidity(state, chainparams, chainstate, *pblock, pindexPrev, false, false)) {
178+
assert(std::addressof(::ChainstateActive()) == std::addressof(m_chainstate));
179+
if (!TestBlockValidity(state, chainparams, m_chainstate, *pblock, pindexPrev, false, false)) {
179180
throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, state.ToString()));
180181
}
181182
int64_t nTime2 = GetTimeMicros();

src/miner.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,7 @@ class BlockAssembler
146146
int64_t nLockTimeCutoff;
147147
const CChainParams& chainparams;
148148
const CTxMemPool& m_mempool;
149+
CChainState& m_chainstate;
149150

150151
public:
151152
struct Options {
@@ -154,11 +155,11 @@ class BlockAssembler
154155
CFeeRate blockMinFeeRate;
155156
};
156157

157-
explicit BlockAssembler(const CTxMemPool& mempool, const CChainParams& params);
158-
explicit BlockAssembler(const CTxMemPool& mempool, const CChainParams& params, const Options& options);
158+
explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params);
159+
explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options);
159160

160161
/** Construct a new block template with coinbase to scriptPubKeyIn */
161-
std::unique_ptr<CBlockTemplate> CreateNewBlock(CChainState& chainstate, const CScript& scriptPubKeyIn);
162+
std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
162163

163164
inline static std::optional<int64_t> m_last_block_num_txs{};
164165
inline static std::optional<int64_t> m_last_block_weight{};
@@ -201,6 +202,7 @@ class BlockAssembler
201202
void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned int& nExtraNonce);
202203
int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
203204

205+
// TODO just accept a CBlockIndex*
204206
/** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */
205207
void RegenerateCommitments(CBlock& block, BlockManager& blockman);
206208

src/net_processing.cpp

Lines changed: 51 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -482,19 +482,70 @@ class PeerManagerImpl final : public PeerManager
482482
/** Offset into vExtraTxnForCompact to insert the next tx */
483483
size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0;
484484

485+
/** Check whether the last unknown block a peer advertised is not yet known. */
485486
void ProcessBlockAvailability(NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
487+
/** Update tracking information about which blocks a peer is assumed to have. */
486488
void UpdateBlockAvailability(NodeId nodeid, const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
487489
bool CanDirectFetch() EXCLUSIVE_LOCKS_REQUIRED(cs_main);
490+
491+
/**
492+
* To prevent fingerprinting attacks, only send blocks/headers outside of
493+
* the active chain if they are no more than a month older (both in time,
494+
* and in best equivalent proof of work) than the best header chain we know
495+
* about and we fully-validated them at some point.
496+
*/
488497
bool BlockRequestAllowed(const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
489498
bool AlreadyHaveBlock(const uint256& block_hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
490499
void ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& inv);
500+
501+
/**
502+
* Validation logic for compact filters request handling.
503+
*
504+
* May disconnect from the peer in the case of a bad request.
505+
*
506+
* @param[in] peer The peer that we received the request from
507+
* @param[in] filter_type The filter type the request is for. Must be basic filters.
508+
* @param[in] start_height The start height for the request
509+
* @param[in] stop_hash The stop_hash for the request
510+
* @param[in] max_height_diff The maximum number of items permitted to request, as specified in BIP 157
511+
* @param[out] stop_index The CBlockIndex for the stop_hash block, if the request can be serviced.
512+
* @param[out] filter_index The filter index, if the request can be serviced.
513+
* @return True if the request can be serviced.
514+
*/
491515
bool PrepareBlockFilterRequest(CNode& peer,
492516
BlockFilterType filter_type, uint32_t start_height,
493517
const uint256& stop_hash, uint32_t max_height_diff,
494518
const CBlockIndex*& stop_index,
495519
BlockFilterIndex*& filter_index);
520+
521+
/**
522+
* Handle a cfilters request.
523+
*
524+
* May disconnect from the peer in the case of a bad request.
525+
*
526+
* @param[in] peer The peer that we received the request from
527+
* @param[in] vRecv The raw message received
528+
*/
496529
void ProcessGetCFilters(CNode& peer, CDataStream& vRecv);
530+
531+
/**
532+
* Handle a cfheaders request.
533+
*
534+
* May disconnect from the peer in the case of a bad request.
535+
*
536+
* @param[in] peer The peer that we received the request from
537+
* @param[in] vRecv The raw message received
538+
*/
497539
void ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv);
540+
541+
/**
542+
* Handle a getcfcheckpt request.
543+
*
544+
* May disconnect from the peer in the case of a bad request.
545+
*
546+
* @param[in] peer The peer that we received the request from
547+
* @param[in] vRecv The raw message received
548+
*/
498549
void ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv);
499550
};
500551
} // namespace
@@ -748,7 +799,6 @@ static bool PeerHasHeader(CNodeState *state, const CBlockIndex *pindex) EXCLUSIV
748799
return false;
749800
}
750801

751-
/** Check whether the last unknown block a peer advertised is not yet known. */
752802
void PeerManagerImpl::ProcessBlockAvailability(NodeId nodeid) {
753803
CNodeState *state = State(nodeid);
754804
assert(state != nullptr);
@@ -764,7 +814,6 @@ void PeerManagerImpl::ProcessBlockAvailability(NodeId nodeid) {
764814
}
765815
}
766816

767-
/** Update tracking information about which blocks a peer is assumed to have. */
768817
void PeerManagerImpl::UpdateBlockAvailability(NodeId nodeid, const uint256 &hash) {
769818
CNodeState *state = State(nodeid);
770819
assert(state != nullptr);
@@ -1191,16 +1240,6 @@ bool PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat
11911240
return false;
11921241
}
11931242

1194-
1195-
//////////////////////////////////////////////////////////////////////////////
1196-
//
1197-
// blockchain -> download logic notification
1198-
//
1199-
1200-
// To prevent fingerprinting attacks, only send blocks/headers outside of the
1201-
// active chain if they are no more than a month older (both in time, and in
1202-
// best equivalent proof of work) than the best header chain we know about and
1203-
// we fully-validated them at some point.
12041243
bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
12051244
{
12061245
AssertLockHeld(cs_main);
@@ -2101,20 +2140,6 @@ void PeerManagerImpl::ProcessOrphanTx(std::set<uint256>& orphan_work_set)
21012140
m_mempool.check(m_chainman.ActiveChainstate());
21022141
}
21032142

2104-
/**
2105-
* Validation logic for compact filters request handling.
2106-
*
2107-
* May disconnect from the peer in the case of a bad request.
2108-
*
2109-
* @param[in] peer The peer that we received the request from
2110-
* @param[in] filter_type The filter type the request is for. Must be basic filters.
2111-
* @param[in] start_height The start height for the request
2112-
* @param[in] stop_hash The stop_hash for the request
2113-
* @param[in] max_height_diff The maximum number of items permitted to request, as specified in BIP 157
2114-
* @param[out] stop_index The CBlockIndex for the stop_hash block, if the request can be serviced.
2115-
* @param[out] filter_index The filter index, if the request can be serviced.
2116-
* @return True if the request can be serviced.
2117-
*/
21182143
bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer,
21192144
BlockFilterType filter_type, uint32_t start_height,
21202145
const uint256& stop_hash, uint32_t max_height_diff,
@@ -2168,14 +2193,6 @@ bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& peer,
21682193
return true;
21692194
}
21702195

2171-
/**
2172-
* Handle a cfilters request.
2173-
*
2174-
* May disconnect from the peer in the case of a bad request.
2175-
*
2176-
* @param[in] peer The peer that we received the request from
2177-
* @param[in] vRecv The raw message received
2178-
*/
21792196
void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv)
21802197
{
21812198
uint8_t filter_type_ser;
@@ -2207,14 +2224,6 @@ void PeerManagerImpl::ProcessGetCFilters(CNode& peer, CDataStream& vRecv)
22072224
}
22082225
}
22092226

2210-
/**
2211-
* Handle a cfheaders request.
2212-
*
2213-
* May disconnect from the peer in the case of a bad request.
2214-
*
2215-
* @param[in] peer The peer that we received the request from
2216-
* @param[in] vRecv The raw message received
2217-
*/
22182227
void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv)
22192228
{
22202229
uint8_t filter_type_ser;
@@ -2259,14 +2268,6 @@ void PeerManagerImpl::ProcessGetCFHeaders(CNode& peer, CDataStream& vRecv)
22592268
m_connman.PushMessage(&peer, std::move(msg));
22602269
}
22612270

2262-
/**
2263-
* Handle a getcfcheckpt request.
2264-
*
2265-
* May disconnect from the peer in the case of a bad request.
2266-
*
2267-
* @param[in] peer The peer that we received the request from
2268-
* @param[in] vRecv The raw message received
2269-
*/
22702271
void PeerManagerImpl::ProcessGetCFCheckPt(CNode& peer, CDataStream& vRecv)
22712272
{
22722273
uint8_t filter_type_ser;

src/node/coin.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
void FindCoins(const NodeContext& node, std::map<COutPoint, Coin>& coins)
1212
{
1313
assert(node.mempool);
14+
assert(node.chainman);
1415
LOCK2(cs_main, node.mempool->cs);
1516
assert(std::addressof(::ChainstateActive()) == std::addressof(node.chainman->ActiveChainstate()));
1617
CCoinsViewCache& chain_view = node.chainman->ActiveChainstate().CoinsTip();

src/node/coinstats.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,11 @@
88

99
#include <amount.h>
1010
#include <uint256.h>
11-
#include <validation.h>
1211

1312
#include <cstdint>
1413
#include <functional>
1514

15+
class BlockManager;
1616
class CCoinsView;
1717

1818
enum class CoinStatsHashType {

0 commit comments

Comments
 (0)