Commit Graph

770 Commits

Author SHA1 Message Date
pasta
a8cb643db8
Merge #6175: feat: allow resigning for EHF
6f82a56171 feat: add option to AsyncSignIfMember to allow signing same requestID on different msgHashes (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  Please see: https://www.dash.org/forum/index.php?threads/ehf-activation-issues.55146/ for a description of this issue

  ## What was done?
  Allow re-signing specifically for EHF messages, this is important as due to rotation, and duplicate requestIDs a member will refuse to re-sign when they participate in a later quorum.

  ## How Has This Been Tested?
  Devnet deployed with a mix of this version, older versions and current v21.0

  ## Breaking Changes
  This doesn't introduce any breaking changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: a758e6363901624d8540983ad09085d47a33fe61e8e576ec4317b051a0f988efb983bd06d9894bcce19c9f587ad703022ce00ceddd3760d7c9d382878f40c2d7
2024-08-07 18:06:48 +07:00
pasta
840175ecb4
Merge #6174: fix: stop trying to sign pending txes when they are no longer non-locked
d8449aecf9 fix: stop trying to sign pending txes when they are no longer non-locked (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  Masternodes keep trying to sign txes they received initially even after these txes were replaced by is/chain-locked txes later.

  ## What was done?
  Remove entries from `pendingRetryTxs` when txes are removed from `nonLockedTxs` set (as either already locked or as conflicted, doesn't matter).

  ## How Has This Been Tested?

  ## Breaking Changes

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK d8449aecf9
  knst:
    utACK d8449aecf9

Tree-SHA512: 45983faad6d590d4e44fc195c77d73d216e3d0bebef963fe556688773c82c07a118916f56fde7f1213df19d4fb9b7d82d8fd51abecd8e6c2108c27d62d0426c1
2024-08-07 15:37:50 +07:00
pasta
024d272eb9
Merge #6126: feat: enable EHF activation of MN_RR on mainnet
a8a3ea0e90 feat: enable EHF activation of MN_RR on mainnet (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  https://github.com/dashpay/dash/issues/6081

  ## What was done?
  Removed a code, that disabled MN_RR activation with EHF on Main Net

  ## How Has This Been Tested?
  This code is tested on devnet, is in process of testing on testnet.

  ## Breaking Changes
  It make MN_RR possible to get active on mainnet.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK a8a3ea0e90
  PastaPastaPasta:
    utACK a8a3ea0e90

Tree-SHA512: 0ae7aecca8463436e952154210cf564333cd77dd1149f7ff88ca144f3b7c434e75e473ea3a6850da1b126efd8a9cece34e46b4bf2b37f5937bcf1f5780e18f50
2024-07-23 12:47:00 -05:00
pasta
9998ffd92b
Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code
0133c9866d feat: add functional test for submitchainlock far ahead in future (Konstantin Akimov)
6004e06769 feat: return enum in RecoveredSig verifying code, apply for RPC submitchainlock (Konstantin Akimov)
130b6d1e96 refactor: replace static private member method to static method (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Currently by result of `submitchainlock` impossible to distinct a situation when a signature is invalid and when a core is far behind and just doesn't know about signing quorum yet.

  This PR aims to fix this issue, as requested by shumkov for needs of platform:

  > mailformed signature and can’t verify signature due to unknown quorum is the same error?
  > possible to distingush ?

  ## What was done?
  Return enum in CL verifying code `chainlock_handler.VerifyChainLock`.
  The RPC `submitchainlock` now returns error with code=-1 and message `no quorum found. Current tip height: {N} hash: {HASH}`

  ## How Has This Been Tested?
  Functional test `feature_llmq_chainlocks.py` is updated

  ## Breaking Changes
  `submitchainlock` return one more error code - not really a breaking change though, because v21 hasn't released yet.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    utACK 0133c9866d
  PastaPastaPasta:
    utACK 0133c9866d

Tree-SHA512: 794ba410efa57aaa66c47a67914deed97c1d060326e5d11a722c9233a8447f5e9215aa4a5ca401cb2199b8fc445144b2b2a692fc35494bf3296a74e9e115bda7
2024-07-15 11:51:48 -05:00
Kittywhiskers Van Gogh
208b1c079b
refactor: drop usage of chainstate globals in masternode logic 2024-06-26 13:50:49 +00:00
Kittywhiskers Van Gogh
303c6bb4db
refactor: drop usage of chainstate globals in llmq logic 2024-06-26 13:50:49 +00:00
Kittywhiskers Van Gogh
c48c0e79f3
refactor: stop using ::ChainstateActive() in GetBlockHash 2024-06-26 11:49:47 +00:00
Kittywhiskers Van Gogh
d7c35d0538
refactor: remove llmq::CQuorumManager global, move to LLMQContext 2024-05-29 08:21:53 +00:00
Kittywhiskers Van Gogh
5b86df6ca3
refactor: reduce llmq::CQuorumManager globals use, use args 2024-05-29 08:20:57 +00:00
Kittywhiskers Van Gogh
1efd219173
refactor: remove llmq::CQuorumBlockProcessor global, move to LLMQContext
We used to store an alias to the `CQuorumBlockProcessor` instance present
in the global state in `LLMQContext` but handled construction and
destruction of the instance in `LLMQContext`. As direct global usage no
longer exists, we can fully subsume it into `LLMQContext`.
2024-05-29 08:17:46 +00:00
Vijay
7b52349b11
Revert "merge #855: Fix bad mn sig ipv6" 2024-05-24 13:33:26 -05:00
pasta
806fc73494
Merge #5954: refactor: significant Mutex refactoring
acd0f49d7b refactor: significant Mutex refactoring (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  Don't use generic names; recursive mutexes where not needed; etc

  ## What was done?
  Includes:
  RecursiveMutex -> Mutex,
  renaming of `cs` to something more meaningful,
  usage of atomics where trivially possible,
  introduce a method CQuorum::SetVerificationVector to avoid needing to lock an internal mutex externally

  ## How Has This Been Tested?
  Compiling

  ## Breaking Changes
  None

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: 76d0ee37e348680bdcd8f03237d3fc1febbf908a9c13e6ddea7be52a35adfca35cde3001ce6ecb140d7dba950ad19519d34d137de17a073306e3e7b26cb95b70
2024-05-14 09:16:33 -05:00
Kittywhiskers Van Gogh
362e3101ad
merge bitcoin#21943: Dedup and RAII-fy the creation of a copy of CConnman::vNodes 2024-05-08 16:21:51 +00:00
Kittywhiskers Van Gogh
bf98ad6a42
merge bitcoin#22782: Remove unused MaybeSetAddrName 2024-05-08 16:20:19 +00:00
pasta
d44b0d5dcb
Merge #5990: refactor: minimize locking in ChainLocks Cleanup
04ba164064 refactor: immediately update lastCleanupTime (pasta)
d0d2641154 refactor: minimize locking of cs_main in chainlocks.cpp (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  Simple changes, just look at the two commits: first we minimize scope of cs_main to what actually needs it. Then we change where we update the `lastCleanupTime` to right after we check it to minimize any chance of two threads entering at the same time.

  ## What was done?

  ## How Has This Been Tested?
  Built, ran for a bit

  ## Breaking Changes
  None

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: 63432ccc16a67e6478e578c96fe809cf33d08e5068285c21e65ccc175c0c9e82c23519d57f0c4ebac162a094bfc20eb44df30cbe85b26815d02eaac06ceb66ad
2024-05-03 10:16:00 -05:00
pasta
acd0f49d7b
refactor: significant Mutex refactoring
Includes:
RecursiveMutex -> Mutex,
renaming of `cs` to something more meaningful,
usage of atomics where trivially possible,
introduce a method CQuorum::SetVerificationVector to avoid needing to lock an internal mutex externally

fix: avoid cs_vvec_shShare double-lock

Co-authored-by: UdjinM6 <udjinm6@users.noreply.github.com>
2024-04-30 12:41:34 -05:00
Kittywhiskers Van Gogh
8b1d3b55ab
llmq: replace LOCKS_EXCLUDED with negative EXCLUSIVE_LOCKS_REQUIRED 2024-04-30 11:47:02 +00:00
Kittywhiskers Van Gogh
33098aefff
merge bitcoin#21160: Move tx inventory into net_processing 2024-04-26 20:25:55 +00:00
Kittywhiskers Van Gogh
4d629a04fb
refactor: limit fMasternodeMode usage in blockstorage, init, net_processing 2024-04-24 18:46:14 +00:00
Kittywhiskers Van Gogh
a9cbdfcebc
refactor: remove fMasternodeMode usage from llmq logic 2024-04-24 18:45:41 +00:00
pasta
04ba164064
refactor: immediately update lastCleanupTime 2024-04-23 11:35:20 -05:00
pasta
d0d2641154
refactor: minimize locking of cs_main in chainlocks.cpp 2024-04-23 11:35:18 -05:00
Kittywhiskers Van Gogh
f2fe39fadc
trivial: add Asserts for m_peerman pointer container uses 2024-04-23 16:08:11 +00:00
Kittywhiskers Van Gogh
35be4e2ebe
llmq: pass PeerManager to llmq::CInstantSendManager constructor
Required to avoid crashes when calling RelayInvFiltered in situations
where the PeerManager* atomic hasn't been set (possible when ProcessMessage
isn't called, leaving the value unset, while a separate thread traverses
the ProcessPendingInstantSendLocks > ProcessPendingInstantSendLocks[1] >
ProcessInstantSendLock > RelayInvFiltered call chain).

[1] - There is a function with the exact same name but with multiple
      arguments
2024-04-23 16:08:11 +00:00
Kittywhiskers Van Gogh
bfd33cd2b4
net: move CConnman::RelayInv{Filtered} into PeerManager 2024-04-23 16:08:10 +00:00
Kittywhiskers Van Gogh
313a7e9a50
trivial: cleanup unnecessary headers in context files 2024-04-23 16:06:41 +00:00
Kittywhiskers Van Gogh
c3f1ac2291
net: retire CConnman::RelayTransaction, use PeerManager::RelayTransaction 2024-04-23 16:06:41 +00:00
Kittywhiskers Van Gogh
0323c6ca17
net: move Relay{Inv, InvFiltered, Transaction} out of CConnman 2024-04-23 16:06:41 +00:00
Kittywhiskers Van Gogh
cf90cf20c6
refactor: remove CMasternodeMetaMan global, move to NodeContext 2024-04-12 17:02:09 +00:00
Kittywhiskers Van Gogh
c99fb42ddf
refactor: remove CMasternodeSync global, move to NodeContext 2024-04-12 17:01:24 +00:00
pasta
54ea9260d4
Merge #5974: backport: bitcoin#19522, #19809, #20993, #21075, #21126, #21138, #21221, #21354, #21542
005a6b104a fix: format string in llmq/commitment - mismatched arguments (Konstantin Akimov)
4774e1e8f6 Merge #19809: log: Prefix log messages with function name and source code location if -logsourcelocations is set (Wladimir J. van der Laan)
43a94f0580 fix: adjust functional tests due to dash's support of thread name after v0.12 (Konstantin Akimov)
085120d9f9 Merge #20993: test: store subversion (user agent) as string in msg_version (MarcoFalke)
e866b43160 Merge #21542: ci: Bump macOS VM image to the latest version (fanquake)
a3702534e5 Merge #21354: build, doc: Drop no longer required packages from macOS cross-compiling dependencies (fanquake)
6bcc86ad3b Merge #21221: [tools] Allow argument/parameter bin packing in clang-format (MarcoFalke)
318c7263d0 Merge #19522: build: fix building libconsensus with reduced exports for Darwin targets (Wladimir J. van der Laan)
88a45d4a9a Merge #21138: ci: Re-run wine tests once if they fail (fanquake)
4abb768456 Merge #21126: ci: Properly bump to focal for win cross build (fanquake)
f254f77d75 Merge #21075: doc: Fix markdown formatting (MarcoFalke)

Pull request description:

  ## Issue being fixed or feature implemented
  Regular backports from bitcoin v22 and related fixes

  ## What was done?
  Follow-up fixes for  bitcoin#19809
  Backports:
   - bitcoin/bitcoin#21075
   - bitcoin/bitcoin#21126
   - bitcoin/bitcoin#21138
   - bitcoin/bitcoin#19522
   - bitcoin/bitcoin#21221
   - bitcoin/bitcoin#21354
   - bitcoin/bitcoin#21542
   - bitcoin/bitcoin#19809
   - bitcoin/bitcoin#20993

  ## How Has This Been Tested?
  Run unit/functional tests

  ## Breaking Changes
  N/A
  Notice, that function name is included now by default to logs with `-logfunctionnames` and many logs have function name twice now such as:
  ```
   node0 2024-04-06T20:13:56.564123Z (mocktime: 2014-12-04T17:15:38Z) [httpworker.3] [masternode/sync.cpp:331] [NotifyHeaderTip] CMasternodeSync::NotifyHeaderTip -- pindexNew->nHeight: 5 fInitialDownload=0
  ```
  For further development need to take it in account and do not use more direct calls `__func__` from code as well as reduce usages in codebase.

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK 005a6b104a

Tree-SHA512: f98949c4605dda7d6dfe790554e1d31a8c8178b57520578fcd58178c68fe7af33c0d66a79865683c1357de9a73fa4f484eb828b28e11ca110b8e1915c1fcf9b3
2024-04-12 10:20:15 -05:00
Konstantin Akimov
005a6b104a
fix: format string in llmq/commitment - mismatched arguments 2024-04-11 02:25:09 +07:00
Wladimir J. van der Laan
4774e1e8f6
Merge #19809: log: Prefix log messages with function name and source code location if -logsourcelocations is set
b4511e2e2ed1a6077ae6826a9ee6b7a311293d08 log: Prefix log messages with function name if -logsourcelocations is set (practicalswift)

Pull request description:

  Prefix log messages with function name if `-logfunctionnames` is set.

  Yes, exactly like `-logthreadnames` but for function names instead of thread names :)

  This is a small developer ergonomics improvement: I've found this to be a cheap/simple way to correlate log output and originating function.

  For me it beats the ordinary cycle of 1.) try to figure out a regexp matching the static part of the dynamic log message, 2.) `git grep -E 'Using .* MiB out of .* requested for signature cache'`, 3.) `mcedit filename.cpp` (`openemacs filename.cpp` works too!) and 4.) search for log message and scroll up to find the function name :)

  Without any logging parameters:

  ```
  $ src/bitcoind -regtest
  2020-08-25T03:29:04Z Using RdRand as an additional entropy source
  2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
  2020-08-25T03:29:04Z Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
  2020-08-25T03:29:04Z Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
  2020-08-25T03:29:04Z block tree size = 1
  2020-08-25T03:29:04Z nBestHeight = 0
  2020-08-25T03:29:04Z Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
  2020-08-25T03:29:04Z 0 addresses found from DNS seeds
  ```

  With `-logthreadnames` and `-logfunctionnames`:

  ```
  $ src/bitcoind -regtest -logthreadnames -logfunctionnames
  2020-08-25T03:29:04Z [init] [ReportHardwareRand] Using RdRand as an additional entropy source
  2020-08-25T03:29:04Z [init] [InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
  2020-08-25T03:29:04Z [init] [InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
  2020-08-25T03:29:04Z [init] [LoadChainTip] Loaded best chain: hashBestChain=0fff88f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e22ff height=0 date=2011-02-02T23:16:42Z progress=1.000000
  2020-08-25T03:29:04Z [init] [AppInitMain] block tree size = 1
  2020-08-25T03:29:04Z [init] [AppInitMain] nBestHeight = 0
  2020-08-25T03:29:04Z [loadblk] [LoadMempool] Imported mempool transactions from disk: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
  2020-08-25T03:29:04Z [dnsseed] [ThreadDNSAddressSeed] 0 addresses found from DNS seeds
  ```

ACKs for top commit:
  laanwj:
    Code review ACK b4511e2e2ed1a6077ae6826a9ee6b7a311293d08
  MarcoFalke:
    review ACK b4511e2e2ed1a6077ae6826a9ee6b7a311293d08 🌃

Tree-SHA512: d100f5364630c323f31d275259864c597f7725e462d5f4bdedcc7033ea616d7fc0d16ef1b2af557e692f4deea73c6773ccfc681589e7bf6ba970b9ec169040c7
2024-04-11 02:25:08 +07:00
Kittywhiskers Van Gogh
91f4588f71
refactor: const the pointer of type const CActiveMasternodeManager 2024-04-09 20:45:32 +00:00
Kittywhiskers Van Gogh
cf940e8d85
init: decouple CMasternodeMetaMan construction from initialization 2024-04-09 20:45:28 +00:00
pasta
27c0813c08
Merge #5953: fix: resolve a few very unlikely lifetime / undefined behavior issues
6f2b350baa fix: don't move out of pendingContributionVerifications; use a ref and then clear (pasta)
7f36f122b2 fix: fix potential mutex lifetime issue were we are returning a reference, and then releasing the mutex (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  See individual commits

  ## What was done?
  Fix potential lifetime issue and potential undefined behavior

  ## How Has This Been Tested?
  Compiling

  ## Breaking Changes
  None

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: f76d88c1fe3c19a92bdf451f147520e29f5edf84342dfb0b6ea9bde901a3f826c09b5aa2334d8f6fa687aaae7d0c109f36779883c670915d55b69af3ea8affd4
2024-04-03 14:11:59 -05:00
Kittywhiskers Van Gogh
2e55327f55
net: introduce CanRelayAddrs as RelayAddrsWithConn substitute
Since bitcoin#21186, mutual exclusivity is not a given (i.e.
RelayAddrsWithConn != !IsBlockOnlyConn), we should use RelayPeersWithConn
for a definitive answer and since relying on a no-longer-true property
breaks InstantSend, let's fetch the right answer instead.
2024-04-03 16:10:17 +00:00
Kittywhiskers Van Gogh
26c39f5b92
net: replace RelayAddrsWithConn check with !IsBlockOnlyConn
Dash uses a lot more CNode::RelayAddrsWithConn checks than Bitcoin (esp.
since a483122f (#4888)), so bitcoin#21186 will not adequately cover the
removal of RelayAddrsWithConn usages.

When possible to query with RelayAddrsWithPeer, that should be used, as
that value is the most reliable, else we rely on the former mutual
exclusivity of IsBlockOnlyConn and RelayAddrsWithConn to fill in the
blanks where a more reliable query isn't available.

Note: To prevent builds from breaking, a change has been made in
InstantSend code despite it breaking functionality. A commit later will
repair it by creating a way to access RelayAddrsWithPeer.
2024-04-03 16:10:17 +00:00
Kittywhiskers Van Gogh
5c4c7c55f8
merge bitcoin#19771: Replace enum CConnMan::NumConnections with enum class ConnectionDirection 2024-04-03 16:10:15 +00:00
pasta
dc6f52ac99
Merge #5961: feat: implement read write locks in threading and use them for CActiveMasternodeManager::cs
069282611c refactor: make CActiveMasternodeManager::cs SharedMutex and private (pasta)
663774c544 feat: implement Read Write Locks in threading (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  We have some caches or other information in codebase which are read from a lot; but rarely written to. We can use a RW lock here instead of a normal Mutex

  ## What was done?
  Implement a RW lock and use them

  ## How Has This Been Tested?
  Hasn't been much; looking for review atm. Maybe should deploy this on testnet for a bit and make sure it doesn't break.
  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK 069282611c

Tree-SHA512: a9759d4904580eebb5ddf9e05d3d54cf4b0b0db971f09d2f4cb093fddc0a13094998ef2af301de581fd64dc1235df80bace7f701ab437c2ecfa663b4fc6e25ed
2024-04-03 10:36:12 -05:00
pasta
6f2b350baa
fix: don't move out of pendingContributionVerifications; use a ref and then clear
Moving out of a variable technically leaves it in an unspecified and potentially invalid state; yet I don't see anywhere we are reseting or otherwise clearing pendingContributionVerifications to ensure it is valid. Instead simply take a ref to it; and then clear it at the end.
2024-04-02 12:05:23 -05:00
pasta
7f36f122b2
fix: fix potential mutex lifetime issue were we are returning a reference, and then releasing the mutex 2024-04-02 12:05:23 -05:00
Konstantin Akimov
ded1b5a3df
fix: deadlock over cs_main and contributionsCacheCs in dkssessionmgr
It fixes rpc failure: "Work queue depth exceeded"
2024-03-29 02:16:32 +07:00
pasta
069282611c
refactor: make CActiveMasternodeManager::cs SharedMutex and private 2024-03-27 20:41:45 -05:00
pasta
f217e0ae7b
Merge #5940: refactor: consolidate activeMasternodeInfo{Cs} into CActiveMasternodeManager, create NodeContext alias, reduce globals usage
815e4f8026 masternode: protect m_{error,state} with cs (pasta)
136e445abc refactor: pass CActiveMasternodeManager as pointer arg to LLMQContext (Kittywhiskers Van Gogh)
5e0f77747a refactor: pass CActiveMasternodeManager as pointer arg to CJContext (Kittywhiskers Van Gogh)
f171c24a29 refactor: add CActiveMasternodeManager NodeContext alias, use in RPC (Kittywhiskers Van Gogh)
44beb941cb refactor: prefix member variable names with m_ (Kittywhiskers Van Gogh)
73cef4f5f9 refactor: make bls{Pub}KeyOperator member variables instead of pointers (Kittywhiskers Van Gogh)
fbc783635a refactor: make m_info private, get const refs (or copies) from Get*() functions (Kittywhiskers Van Gogh)
1b516ce4ed refactor: use signing helper function instead of passing blsKeyOperator (Kittywhiskers Van Gogh)
33702aca39 refactor: add helper function to decrypt messages with blsKeyOperator (Kittywhiskers Van Gogh)
3eb931b596 refactor: add helper function to sign messages with blsKeyOperator (Kittywhiskers Van Gogh)
3827355cce refactor: move key initialization to InitKeys, define destructor (Kittywhiskers Van Gogh)
e5295dec1f refactor: move activeMasternodeInfo{Cs} into CActiveMasternodeManager (Kittywhiskers Van Gogh)
b8c1f010e7 refactor: avoid accessing active masternode info if not in masternode mode (Kittywhiskers Van Gogh)
9a3c5a3c48 trivial: access activeMasternodeInfo when lock is in scope (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * `CActiveMasternodeManager`, unlike other managers, is _conditionally_ initialized (specifically, when the node is hosting a masternode). This means that checks need to be made to ensure that the conditions needed to initialize the manager are true or that the pointer leads to a valid manager instance.

    As the codebase currently checks (and fast-fails) based on the node being in "masternode mode" (`fMasternodeMode`) or not, we will continue with this approach, but with additional assertions _after_ the masternode mode check if the manager exists.

  * Though, since `activeMasternodeInfo`(`Cs`) are global variables, they can be accessed _regardless_ of whether the corresponding manager exists. This means some parts of the codebase attempt to fetch information about the (nonexistent) active masternode _before_ determining if it should use the masternode mode path or not (looking at you, `CMNAuth::ProcessMessage`)

    Moving them into `CActiveMasternodeManager` meant adding checks _before_ attempting to access information about the masternode, as they would no longer be accessible with dummy values ([here](2110c0c309/src/init.cpp (L1633-L1635))) on account of being part of the conditionally initialized manager.
    * In an attempt to opportunistically dereference the manager, `CDKGSessionManager` (accepting a pointer) was dereferencing the manager before passing it to `CDKGSessionHandler`. This was done under the assumption that  `CDKGSessionManager` would only ever be initialized in masternode mode.

      This is not true. I can confirm that because I spent a few days trying to debug test failures. `CDKGSessionHandler` is initialized in two scenarios:

      * In masternode mode
      * If the `-watchquorums` flag is enabled

      The latter scenario doesn't initialize `CActiveMasternodeManager`.

      Furthermore, the DKG round thread is started unconditionally ([here](2110c0c309/src/llmq/context.cpp (L79))) and the `CDKGSessionHandler::StartThreads` > `CDKGSessionHandler::StartThread` > `CDKGSessionHandler::PhaseHandlerThread` > `CDKGSessionHandler::HandleDKGRound` > `CDKGSessionHandler::InitNewQuorum` > `CActiveMasternodeManager::GetProTxHash` call chain reveals an attempt to fetch active masternode information without any masternode mode checks.

      This behaviour has now been changed and the thread will only be spun up if in masternode mode.

    * Dereferencing so far has been limited to objects that primarily hold data (like `CCoinJoinBroadcastTx` or `CGovernanceObject`) as they should not have knowledge of node's state (that responsibility lies with whatever manager manipulates those objects), perform one-off operations and static functions.

  * `activeMasternodeInfo` allowed its members to be read-write accessible to anybody who asked. Additionally, signing and decrypting involved borrowing the operator secret key from the active masternode state to perform those operations.

     This behaviour has now been changed. The internal state is now private and accessible read-only as a const ref (or copy) and `Decrypt`/`Sign` functions have been implemented to allow those operations to happen without having another manager access the operator private key in order to do so.

  * You cannot combine a `WITH_LOCK` and an `Assert` (in either mutex or accessed value), doing so will cause errors if `-Werror=thread-safety` is enabled. This is why `assert`s are added even when it would intuitively seem that `Assert` would've been more appropriate to use.

  ## Future Considerations

  Currently there are no unit tests that test the functionality of `CActiveMasternodeManager` as it's never initialized in test contexts, breakage had to be found using functional tests. Perhaps some (rudimentary) tests for `CActiveMasternodeManager` may prove to be valuable.

  ## Breaking Changes

  Not _really_. Some behaviour has been modified but nothing that should necessitate updates or upgrades.

  ## Checklist:

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 815e4f8026

Tree-SHA512: cbe49ea9e1c35df514e1b40869ee271baef1c348c9d09e4b356e5fc8fe5449cbbe66569258f2d664029faa9a46f711df9bf9e41eb8734c3aefc6cd8e94378948
2024-03-26 08:43:54 -05:00
pasta
9201529b8e
Merge #5955: refactor: simply RecursiveMutex -> Mutex
a09e260c15 refacotor: simply RecursiveMutex -> Mutex (pasta)

Pull request description:

  ## What was done?
  This PR is a simpler version of https://github.com/dashpay/dash/pull/5954; it is simply a recursiveMutex -> Mutex PR; as I think some of the scope minimization in the other PR introduced test failures (appears there are undocumented mutex dependancies)

  ## How Has This Been Tested?
  CI TBD

  ## Breaking Changes
  None

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK a09e260c15

Tree-SHA512: 4ef5aa94ecb1a55f30b642d5e0328e2d2a0adcf2ccc2ab6489a576a420c915339be023828785c3243f019bb73ac2cd77be7dd4397e14ec874cba68999a9e54dc
2024-03-25 11:06:07 -05:00
pasta
a09e260c15
refacotor: simply RecursiveMutex -> Mutex 2024-03-24 23:14:21 -05:00
Kittywhiskers Van Gogh
136e445abc
refactor: pass CActiveMasternodeManager as pointer arg to LLMQContext 2024-03-24 07:37:31 +00:00
Kittywhiskers Van Gogh
fbc783635a
refactor: make m_info private, get const refs (or copies) from Get*() functions
External logic should not be able to mutate the CActiveMasternodeManager
state (i.e. CActiveMasternodeInfo). Access is brokered through getter
functions.
2024-03-24 07:37:29 +00:00
Kittywhiskers Van Gogh
33702aca39
refactor: add helper function to decrypt messages with blsKeyOperator 2024-03-24 07:20:58 +00:00