Skip to content

Commit 886024b

Browse files
UdjinM6UdjinM6
authored
Merge pull request dashpay#4203 from UdjinM6/pr4196
ci: Add `--enable-werror` to arm and c++17 builds (and fix all issues found via these builds)
2 parents 0dc8cbc + d1ff298 commit 886024b

33 files changed

+117
-107
lines changed

‎ci/matrix.sh

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ if [ "$BUILD_TARGET" = "arm-linux" ]; then
4040
export CHECK_DOC=1
4141
# -Wno-psabi is to disable ABI warnings: "note: parameter passing for argument of type ... changed in GCC 7.1"
4242
# This could be removed once the ABI change warning does not show up by default
43-
export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports CXXFLAGS=-Wno-psabi"
43+
export BITCOIN_CONFIG="--enable-glibc-back-compat --enable-reduce-exports --enable-werror CXXFLAGS=-Wno-psabi"
4444
export RUN_UNITTESTS=false
4545
export RUN_INTEGRATIONTESTS=false
4646
elif [ "$BUILD_TARGET" = "win32" ]; then
@@ -67,7 +67,7 @@ elif [ "$BUILD_TARGET" = "linux64" ]; then
6767
elif [ "$BUILD_TARGET" = "linux64_cxx17" ]; then
6868
export HOST=x86_64-unknown-linux-gnu
6969
export DEP_OPTS="NO_UPNP=1 DEBUG=1"
70-
export BITCOIN_CONFIG="--enable-zmq --enable-glibc-back-compat --enable-reduce-exports --enable-crash-hooks --enable-c++17 --with-sanitizers=undefined"
70+
export BITCOIN_CONFIG="--enable-zmq --enable-glibc-back-compat --enable-reduce-exports --enable-crash-hooks --enable-c++17 --enable-werror --with-sanitizers=undefined"
7171
export CPPFLAGS="-DDEBUG_LOCKORDER -DENABLE_DASH_DEBUG -DARENA_DEBUG"
7272
export PYZMQ=true
7373
export RUN_INTEGRATIONTESTS=false

‎src/bench/bench_dash.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020

2121
const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
2222

23-
static const int64_t DEFAULT_BENCH_EVALUATIONS = 5;
2423
static const char* DEFAULT_BENCH_FILTER = ".*";
2524

2625
void InitBLSTests();

‎src/bench/bls_dkg.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,6 @@ class DKG
8686
ReceiveVvecs();
8787
size_t memberIdx = 0;
8888
bench.minEpochIterations(epoch_iters).run([&] {
89-
auto& m = members[memberIdx];
90-
9189
ReceiveShares(memberIdx);
9290

9391
std::set<size_t> invalidIndexes;

‎src/bench/checkqueue.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@
1111
#include <random.h>
1212

1313

14-
static const int MIN_CORES = 2;
1514
static const size_t BATCHES = 101;
1615
static const size_t BATCH_SIZE = 30;
1716
static const int PREVECTOR_SIZE = 28;

‎src/hdchain.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,7 @@ class CHDChain
7474

7575
// by swapping the members of two classes,
7676
// the two classes are effectively swapped
77+
LOCK2(first.cs, second.cs);
7778
swap(first.nVersion, second.nVersion);
7879
swap(first.id, second.id);
7980
swap(first.fCrypted, second.fCrypted);

‎src/init.cpp

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2061,7 +2061,6 @@ bool AppInitMain()
20612061
LogPrintf("* Using %.1fMiB for in-memory UTXO set (plus up to %.1fMiB of unused mempool space)\n", nCoinCacheUsage * (1.0 / 1024 / 1024), nMempoolSizeMax * (1.0 / 1024 / 1024));
20622062

20632063
bool fLoaded = false;
2064-
int64_t nStart = GetTimeMillis();
20652064

20662065
while (!fLoaded && !ShutdownRequested()) {
20672066
bool fReset = fReindex;

‎src/llmq/quorums_chainlocks.cpp

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -555,23 +555,24 @@ void CChainLocksHandler::EnforceBestChainLock()
555555
activateNeeded = chainActive.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) != currentBestChainLockBlockIndex;
556556
}
557557

558-
if (activateNeeded && !ActivateBestChain(state, params)) {
559-
LogPrintf("CChainLocksHandler::%s -- ActivateBestChain failed: %s\n", __func__, FormatStateMessage(state));
560-
}
561-
562-
const CBlockIndex* pindexNotify = nullptr;
563-
{
558+
if (activateNeeded) {
559+
if(!ActivateBestChain(state, params)) {
560+
LogPrintf("CChainLocksHandler::%s -- ActivateBestChain failed: %s\n", __func__, FormatStateMessage(state));
561+
return;
562+
}
564563
LOCK(cs_main);
565-
if (lastNotifyChainLockBlockIndex != currentBestChainLockBlockIndex &&
566-
chainActive.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) == currentBestChainLockBlockIndex) {
567-
lastNotifyChainLockBlockIndex = currentBestChainLockBlockIndex;
568-
pindexNotify = currentBestChainLockBlockIndex;
564+
if (chainActive.Tip()->GetAncestor(currentBestChainLockBlockIndex->nHeight) != currentBestChainLockBlockIndex) {
565+
return;
569566
}
570567
}
571568

572-
if (pindexNotify) {
573-
GetMainSignals().NotifyChainLock(pindexNotify, clsig);
569+
{
570+
LOCK(cs);
571+
if (lastNotifyChainLockBlockIndex == currentBestChainLockBlockIndex) return;
572+
lastNotifyChainLockBlockIndex = currentBestChainLockBlockIndex;
574573
}
574+
575+
GetMainSignals().NotifyChainLock(currentBestChainLockBlockIndex, clsig);
575576
}
576577

577578
void CChainLocksHandler::HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig)

‎src/llmq/quorums_dkgsession.cpp

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,8 @@ void CDKGSession::VerifyAndJustify(CDKGPendingMessages& pendingMessages)
657657

658658
std::set<uint256> justifyFor;
659659

660+
LOCK(invCs);
661+
660662
for (const auto& m : members) {
661663
if (m->bad) {
662664
continue;
@@ -1212,21 +1214,25 @@ std::vector<CFinalCommitment> CDKGSession::FinalizeCommitments()
12121214
typedef std::vector<bool> Key;
12131215
std::map<Key, std::vector<CDKGPrematureCommitment>> commitmentsMap;
12141216

1215-
for (const auto& p : prematureCommitments) {
1216-
auto& qc = p.second;
1217-
if (!validCommitments.count(p.first)) {
1218-
continue;
1219-
}
1217+
{
1218+
LOCK(invCs);
12201219

1221-
// should have been verified before
1222-
assert(qc.CountValidMembers() >= params.minSize);
1220+
for (const auto& p : prematureCommitments) {
1221+
auto& qc = p.second;
1222+
if (!validCommitments.count(p.first)) {
1223+
continue;
1224+
}
12231225

1224-
auto it = commitmentsMap.find(qc.validMembers);
1225-
if (it == commitmentsMap.end()) {
1226-
it = commitmentsMap.emplace(qc.validMembers, std::vector<CDKGPrematureCommitment>()).first;
1227-
}
1226+
// should have been verified before
1227+
assert(qc.CountValidMembers() >= params.minSize);
12281228

1229-
it->second.emplace_back(qc);
1229+
auto it = commitmentsMap.find(qc.validMembers);
1230+
if (it == commitmentsMap.end()) {
1231+
it = commitmentsMap.emplace(qc.validMembers, std::vector<CDKGPrematureCommitment>()).first;
1232+
}
1233+
1234+
it->second.emplace_back(qc);
1235+
}
12301236
}
12311237

12321238
std::vector<CFinalCommitment> finalCommitments;

‎src/llmq/quorums_dkgsessionhandler.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,7 @@ std::set<NodeId> BatchVerifyMessageSigs(CDKGSession& session, const std::vector<
386386
}
387387

388388
// are all messages from the same node?
389-
NodeId firstNodeId;
389+
NodeId firstNodeId{-1};
390390
first = true;
391391
bool nodeIdsAllSame = true;
392392
for (auto it = messages.begin(); it != messages.end(); ++it) {

‎src/llmq/quorums_dkgsessionhandler.h

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -117,10 +117,11 @@ class CDKGSessionHandler
117117
std::shared_ptr<CDKGSession> curSession;
118118
std::thread phaseHandlerThread;
119119

120-
CDKGPendingMessages pendingContributions GUARDED_BY(cs);
121-
CDKGPendingMessages pendingComplaints GUARDED_BY(cs);
122-
CDKGPendingMessages pendingJustifications GUARDED_BY(cs);
123-
CDKGPendingMessages pendingPrematureCommitments GUARDED_BY(cs);
120+
// Do not guard these, they protect their internals themselves
121+
CDKGPendingMessages pendingContributions;
122+
CDKGPendingMessages pendingComplaints;
123+
CDKGPendingMessages pendingJustifications;
124+
CDKGPendingMessages pendingPrematureCommitments;
124125

125126
public:
126127
CDKGSessionHandler(const Consensus::LLMQParams& _params, CBLSWorker& blsWorker, CDKGSessionManager& _dkgManager);

‎src/llmq/quorums_instantsend.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1418,7 +1418,7 @@ void CInstantSendManager::AskNodesForLockedTx(const uint256& txid)
14181418
{
14191419
std::vector<CNode*> nodesToAskFor;
14201420
g_connman->ForEachNode([&](CNode* pnode) {
1421-
LOCK(pnode->cs_filter);
1421+
LOCK(pnode->cs_inventory);
14221422
if (pnode->filterInventoryKnown.contains(txid)) {
14231423
pnode->AddRef();
14241424
nodesToAskFor.emplace_back(pnode);

‎src/llmq/quorums_signing_shares.cpp

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -594,6 +594,7 @@ void CSigSharesManager::CollectPendingSigSharesToVerify(
594594
}
595595
auto& sigShare = *ns.pendingIncomingSigShares.GetFirst();
596596

597+
AssertLockHeld(cs);
597598
bool alreadyHave = this->sigShares.Has(sigShare.GetKey());
598599
if (!alreadyHave) {
599600
uniqueSignHashes.emplace(nodeId, sigShare.GetSignHash());
@@ -1062,6 +1063,7 @@ void CSigSharesManager::CollectSigSharesToAnnounce(std::unordered_map<NodeId, st
10621063
std::unordered_map<std::pair<Consensus::LLMQType, uint256>, std::unordered_set<NodeId>, StaticSaltedHasher> quorumNodesMap;
10631064

10641065
sigSharesQueuedToAnnounce.ForEach([&](const SigShareKey& sigShareKey, bool) {
1066+
AssertLockHeld(cs);
10651067
auto& signHash = sigShareKey.first;
10661068
auto quorumMember = sigShareKey.second;
10671069
const CSigShare* sigShare = sigShares.Get(sigShareKey);
@@ -1421,6 +1423,7 @@ void CSigSharesManager::Cleanup()
14211423
}
14221424
// remove global requested state to force a re-request from another node
14231425
it->second.requestedSigShares.ForEach([&](const SigShareKey& k, bool) {
1426+
AssertLockHeld(cs);
14241427
sigSharesRequested.Erase(k);
14251428
});
14261429
nodeStates.erase(nodeId);
@@ -1455,6 +1458,7 @@ void CSigSharesManager::RemoveBannedNodeStates()
14551458
if (IsBanned(it->first)) {
14561459
// re-request sigshares from other nodes
14571460
it->second.requestedSigShares.ForEach([&](const SigShareKey& k, int64_t) {
1461+
AssertLockHeld(cs);
14581462
sigSharesRequested.Erase(k);
14591463
});
14601464
it = nodeStates.erase(it);
@@ -1484,6 +1488,7 @@ void CSigSharesManager::BanNode(NodeId nodeId)
14841488

14851489
// Whatever we requested from him, let's request it from someone else now
14861490
nodeState.requestedSigShares.ForEach([&](const SigShareKey& k, int64_t) {
1491+
AssertLockHeld(cs);
14871492
sigSharesRequested.Erase(k);
14881493
});
14891494
nodeState.requestedSigShares.Clear();

‎src/masternode/masternode-sync.cpp

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,20 +23,24 @@ CMasternodeSync::CMasternodeSync()
2323
void CMasternodeSync::Reset(bool fForce, bool fNotifyReset)
2424
{
2525
// Avoid resetting the sync process if we just "recently" received a new block
26-
if (fForce || (GetTime() - nTimeLastUpdateBlockTip > MASTERNODE_SYNC_RESET_SECONDS)) {
27-
{
28-
LOCK(cs);
29-
nCurrentAsset = MASTERNODE_SYNC_BLOCKCHAIN;
30-
nTriedPeerCount = 0;
31-
nTimeAssetSyncStarted = GetTime();
32-
nTimeLastBumped = GetTime();
33-
nTimeLastUpdateBlockTip = 0;
34-
fReachedBestHeader = false;
35-
}
36-
if (fNotifyReset) {
37-
uiInterface.NotifyAdditionalDataSyncProgressChanged(-1);
26+
if (!fForce) {
27+
LOCK(cs);
28+
if (GetTime() - nTimeLastUpdateBlockTip < MASTERNODE_SYNC_RESET_SECONDS) {
29+
return;
3830
}
3931
}
32+
{
33+
LOCK(cs);
34+
nCurrentAsset = MASTERNODE_SYNC_BLOCKCHAIN;
35+
nTriedPeerCount = 0;
36+
nTimeAssetSyncStarted = GetTime();
37+
nTimeLastBumped = GetTime();
38+
nTimeLastUpdateBlockTip = 0;
39+
fReachedBestHeader = false;
40+
}
41+
if (fNotifyReset) {
42+
uiInterface.NotifyAdditionalDataSyncProgressChanged(-1);
43+
}
4044
}
4145

4246
void CMasternodeSync::BumpAssetLastTime(const std::string& strFuncName)
@@ -88,6 +92,7 @@ void CMasternodeSync::SwitchToNextAsset(CConnman& connman)
8892

8993
std::string CMasternodeSync::GetSyncStatus() const
9094
{
95+
LOCK(cs);
9196
switch (nCurrentAsset) {
9297
case MASTERNODE_SYNC_BLOCKCHAIN: return _("Synchronizing blockchain...");
9398
case MASTERNODE_SYNC_GOVERNANCE: return _("Synchronizing governance objects...");
@@ -143,6 +148,7 @@ void CMasternodeSync::ProcessTick(CConnman& connman)
143148
return;
144149
}
145150

151+
LOCK(cs);
146152
// Calculate "progress" for LOG reporting / GUI notification
147153
double nSyncProgress = double(nTriedPeerCount + (nCurrentAsset - 1) * 8) / (8*4);
148154
LogPrint(BCLog::MNSYNC, "CMasternodeSync::ProcessTick -- nTick %d nCurrentAsset %d nTriedPeerCount %d nSyncProgress %f\n", nTick, nCurrentAsset, nTriedPeerCount, nSyncProgress);
@@ -371,17 +377,15 @@ void CMasternodeSync::UpdatedBlockTip(const CBlockIndex *pindexNew, bool fInitia
371377
// Note: since we sync headers first, it should be ok to use this
372378
bool fReachedBestHeaderNew = pindexNew->GetBlockHash() == pindexTip->GetBlockHash();
373379

380+
LOCK(cs);
374381
if (fReachedBestHeader && !fReachedBestHeaderNew) {
375382
// Switching from true to false means that we previously stuck syncing headers for some reason,
376383
// probably initial timeout was not enough,
377384
// because there is no way we can update tip not having best header
378385
Reset(true);
379386
}
380387

381-
{
382-
LOCK(cs);
383-
fReachedBestHeader = fReachedBestHeaderNew;
384-
}
388+
fReachedBestHeader = fReachedBestHeaderNew;
385389
LogPrint(BCLog::MNSYNC, "CMasternodeSync::UpdatedBlockTip -- pindexNew->nHeight: %d pindexTip->nHeight: %d fInitialDownload=%d fReachedBestHeader=%d\n",
386390
pindexNew->nHeight, pindexTip->nHeight, fInitialDownload, fReachedBestHeader);
387391
}

‎src/net.cpp

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1278,15 +1278,15 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
12781278
m_msgproc->InitializeNode(pnode);
12791279

12801280
if (fLogIPs) {
1281-
LogPrint(BCLog::NET_NETCONN, "connection from %s accepted, sock=%d, peer=%d\n", addr.ToString(), pnode->hSocket, pnode->GetId());
1281+
LogPrint(BCLog::NET_NETCONN, "connection from %s accepted, sock=%d, peer=%d\n", addr.ToString(), hSocket, pnode->GetId());
12821282
} else {
1283-
LogPrint(BCLog::NET_NETCONN, "connection accepted, sock=%d, peer=%d\n", pnode->hSocket, pnode->GetId());
1283+
LogPrint(BCLog::NET_NETCONN, "connection accepted, sock=%d, peer=%d\n", hSocket, pnode->GetId());
12841284
}
12851285

12861286
{
12871287
LOCK(cs_vNodes);
12881288
vNodes.push_back(pnode);
1289-
mapSocketToNode.emplace(pnode->hSocket, pnode);
1289+
mapSocketToNode.emplace(hSocket, pnode);
12901290
RegisterEvents(pnode);
12911291
WakeSelect();
12921292
}
@@ -1443,8 +1443,9 @@ void CConnman::CalculateNumConnectionsChangedStats()
14431443
}
14441444
mapRecvBytesMsgStats[NET_MESSAGE_COMMAND_OTHER] = 0;
14451445
mapSentBytesMsgStats[NET_MESSAGE_COMMAND_OTHER] = 0;
1446-
LOCK(cs_vNodes);
1447-
for (const CNode* pnode : vNodes) {
1446+
auto vNodesCopy = CopyNodeVector(CConnman::FullyConnectedOnly);
1447+
for (auto pnode : vNodesCopy) {
1448+
LOCK(pnode->cs_vRecv);
14481449
for (const mapMsgCmdSize::value_type &i : pnode->mapRecvBytesPerMsgCmd)
14491450
mapRecvBytesMsgStats[i.first] += i.second;
14501451
for (const mapMsgCmdSize::value_type &i : pnode->mapSendBytesPerMsgCmd)
@@ -1466,6 +1467,7 @@ void CConnman::CalculateNumConnectionsChangedStats()
14661467
if(pnode->nPingUsecTime > 0)
14671468
statsClient.timing("peers.ping_us", pnode->nPingUsecTime, 1.0f);
14681469
}
1470+
ReleaseNodeVector(vNodesCopy);
14691471
for (const std::string &msg : getAllNetMessageTypes()) {
14701472
statsClient.gauge("bandwidth.message." + msg + ".totalBytesReceived", mapRecvBytesMsgStats[msg], 1.0f);
14711473
statsClient.gauge("bandwidth.message." + msg + ".totalBytesSent", mapSentBytesMsgStats[msg], 1.0f);
@@ -2787,7 +2789,12 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
27872789
LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- ConnectNode failed for %s\n", __func__, getIpStr());
27882790
return;
27892791
}
2790-
LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- succesfully connected to %s, sock=%d, peer=%d\n", __func__, getIpStr(), pnode->hSocket, pnode->GetId());
2792+
2793+
{
2794+
LOCK(pnode->cs_hSocket);
2795+
LogPrint(BCLog::NET_NETCONN, "CConnman::%s -- succesfully connected to %s, sock=%d, peer=%d\n", __func__, getIpStr(), pnode->hSocket, pnode->GetId());
2796+
}
2797+
27912798
if (grantOutbound)
27922799
grantOutbound->MoveTo(pnode->grantOutbound);
27932800
if (fOneShot)
@@ -2802,7 +2809,7 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
28022809
pnode->m_masternode_probe_connection = true;
28032810

28042811
{
2805-
LOCK(cs_vNodes);
2812+
LOCK2(cs_vNodes, pnode->cs_hSocket);
28062813
mapSocketToNode.emplace(pnode->hSocket, pnode);
28072814
}
28082815

@@ -3374,7 +3381,10 @@ void CConnman::Stop()
33743381
}
33753382
vNodes.clear();
33763383
mapSocketToNode.clear();
3377-
mapReceivableNodes.clear();
3384+
{
3385+
LOCK(cs_vNodes);
3386+
mapReceivableNodes.clear();
3387+
}
33783388
{
33793389
LOCK(cs_mapNodesWithDataToSend);
33803390
mapNodesWithDataToSend.clear();

‎src/net_processing.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1077,7 +1077,6 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV
10771077
}
10781078
}
10791079

1080-
// Requires cs_main.
10811080
bool IsBanned(NodeId pnode)
10821081
{
10831082
CNodeState *state = State(pnode);
@@ -4235,13 +4234,13 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
42354234
//
42364235
std::vector<CInv> vInv;
42374236
{
4237+
LOCK2(mempool.cs, pto->cs_inventory);
4238+
42384239
size_t reserve = std::min<size_t>(pto->setInventoryTxToSend.size(), INVENTORY_BROADCAST_MAX_PER_1MB_BLOCK * MaxBlockSize() / 1000000);
42394240
reserve = std::max<size_t>(reserve, pto->vInventoryBlockToSend.size());
42404241
reserve = std::min<size_t>(reserve, MAX_INV_SZ);
42414242
vInv.reserve(reserve);
42424243

4243-
LOCK2(mempool.cs, pto->cs_inventory);
4244-
42454244
// Add blocks
42464245
for (const uint256& hash : pto->vInventoryBlockToSend) {
42474246
vInv.push_back(CInv(MSG_BLOCK, hash));

0 commit comments

Comments
 (0)