69cf5d4eeb73f7d685e915fc17af64634d88a4a2 [test] Make sure send rpc returns fee reason (Sishir Giri)
d5863c0b3e20d56acf7246008b7832efde68ab21 [send] Make send RPCs return fee reason (Sishir Giri)
Pull request description:
Whenever a wallet funds a transaction, the fee reason is reported to the user only if the verbose is set to true. I added an extra parameter to `CreateTransaction` function in wallet.cpp. Then I implemented the fee reason return logic in `SendMoney` in rpcwallet.cpp, followed by verbose parameter in `sendtoaddress` and `sendmany` functions. I also added a fee reason test case in walletbasic.py.
link to the issue: https://github.com/MarcoFalke/bitcoin-core/issues/22#issue-616251578
ACKs for top commit:
instagibbs:
ACK 69cf5d4eeb
meshcollider:
utACK 69cf5d4eeb73f7d685e915fc17af64634d88a4a2
Tree-SHA512: 2e3af32dcfbd5511ba95f8bc8edca7acfe709a8430ff03e43172e5d0af3dfa4b2f57906978e7f272d878043b9ed8c6004674cf47d7496b005d5f612e9a58aa0e
b2910fba02 fix: resolve potential deadlocks in CJ (UdjinM6)
Pull request description:
## Issue being fixed or feature implemented
```
POTENTIAL DEADLOCK DETECTED
Previous lock order was:
(2) 'cs_wallet' in wallet/wallet.cpp:3826 (in thread 'qt-init')
(2) 'pwallet->cs_wallet' in wallet/walletdb.cpp:705 (in thread 'qt-init')
(1) 'cs_KeyStore' in wallet/scriptpubkeyman.cpp:971 (in thread 'qt-init')
Current lock order is:
'cs_deqsessions' in coinjoin/client.cpp:261 (in thread 'main')
(1) 'cs_KeyStore' in wallet/scriptpubkeyman.cpp:1522 (in thread 'main')
(2) 'cs_wallet' in wallet/wallet.cpp:1629 (in thread 'main')
```
This one is for `ResetPool()`.
## What was done?
Lock `cs_wallet` when calling `keyHolderStorage.ReturnAll()` in some places* to ensure the right order of locks.
(*In other places `cs_wallet` is held already, no need to double lock).
## How Has This Been Tested?
Mixing
## Breaking Changes
n/a
## 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 b2910fba02
Tree-SHA512: 8be98df021f7683cd496ebe095dd7b32ebea76c7f9c7c085af3bc0a6901d9cfd4d4624e20a2eee1f3b0d69fd711f8fceb9a91c386b9bf02475632a23b3a0f09a
```
POTENTIAL DEADLOCK DETECTED
Previous lock order was:
(2) 'cs_wallet' in wallet/wallet.cpp:3826 (in thread 'qt-init')
(2) 'pwallet->cs_wallet' in wallet/walletdb.cpp:705 (in thread 'qt-init')
(1) 'cs_KeyStore' in wallet/scriptpubkeyman.cpp:971 (in thread 'qt-init')
Current lock order is:
'cs_deqsessions' in coinjoin/client.cpp:261 (in thread 'main')
(1) 'cs_KeyStore' in wallet/scriptpubkeyman.cpp:1522 (in thread 'main')
(2) 'cs_wallet' in wallet/wallet.cpp:1629 (in thread 'main')
```
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
fac39c198324715565897f4240709340477af0bf wallet: document that tx in CreateTransaction is purely an out-param (MarcoFalke)
faac31521bb7ecbf999541cf918d3750ff589de4 Remove unused and confusing CTransaction constructor (MarcoFalke)
Pull request description:
The constructor is confusing and dangerous (as explained in the TODO), fix that by removing it.
ACKs for top commit:
laanwj:
Code review ACK fac39c198324715565897f4240709340477af0bf
promag:
Code review ACK fac39c198324715565897f4240709340477af0bf.
theStack:
Code review ACK fac39c198324715565897f4240709340477af0bf
Tree-SHA512: e0c8cffce8d8ee0166b8e1cbfe85ed0657611e26e2af0d69fde70eceaa5d75cbde3eb489af0428fe4fc431360b4c791fb1cc21b8dee7d4c7a4f17df00836229d
We could use std::optional<std::reference_wrapper<const CActiveMasternodeManager>>
but then we'd also have to contend with accessing the value with mn_activeman.
value().get().
We assert m_mn_activeman is present instead of fast-failing when it isn't
because of the fMasternodeMode fast-fail check. If we are in masternode
mode, m_mn_activeman should point to a valid target. If it doesn't,
something's gone wrong.
External logic should not be able to mutate the CActiveMasternodeManager
state (i.e. CActiveMasternodeInfo). Access is brokered through getter
functions.
that's a result of:
contrib/devtools/copyright_header.py update ./
it is not scripted diff, because it works differentlly on my localhost and in CI:
CI doesn't want to use git commit date which is mocked to 30th Dec of 2023
ddefb5c0b759950942ac03f28c43b548af7b4033 p2p: Use the greatest common version in peer logic (Hennadii Stepanov)
e084d45562b94827b3a7873895882fcaae9f4d48 p2p: Remove SetCommonVersion() from VERACK handler (Hennadii Stepanov)
8d2026796a6f7add0c2cda9806e759817d1eae6f refactor: Rename local variable nSendVersion (Hennadii Stepanov)
e9a6d8b13b0558b17cdafbd32fd2663b4138ff11 p2p: Unify Send and Receive protocol versions (Hennadii Stepanov)
Pull request description:
On master (6fef85bfa3cd7f76e83b8b57f9e4acd63eb664ec) `CNode` has two members to keep protocol version:
- `nRecvVersion` for received messages
- `nSendVersion` for messages to send
After exchanging with `VERSION` and `VERACK` messages via protocol version `INIT_PROTO_VERSION`, both nodes set `nRecvVersion` _and_ `nSendVersion` to _the same_ value which is the greatest common protocol version.
This PR:
- replaces two `CNode` members, `nRecvVersion` `nSendVersion`, with `m_greatest_common_version`
- removes duplicated getter and setter
There is no change in behavior on the P2P network.
ACKs for top commit:
jnewbery:
ACK ddefb5c0b759950942ac03f28c43b548af7b4033
naumenkogs:
ACK ddefb5c0b759950942ac03f28c43b548af7b4033
fjahr:
Code review ACK ddefb5c0b759950942ac03f28c43b548af7b4033
amitiuttarwar:
code review but untested ACK ddefb5c0b7
benthecarman:
utACK `ddefb5c`
Tree-SHA512: 5305538dbaa5426b923b0afd20bdef4f248d310855d1d78427210c00716c67b7cb691515c421716b6157913e453076e293b10ff5fd2cd26a8e5375d42da7809d
## Issue being fixed or feature implemented
`llmq/utils` has simple util code that used all over code base and also
have too heavy code for calculation quorums such as:
`GetAllQuorumMembers`, `EnsureQuorumConnections` and other.
These helpers for calculation quorums are used only by
evo/deterministicmns, evo/simplifiedmns and llmq/* modules, but
llmq/utils is included in many other modules for various trivial
helpers.
## What was done?
Prior work:
- https://github.com/dashpay/dash/pull/5753
- #5486
See also #4798
This PR remove all non-quorum calculation code from llmq/utils.
Eventually it happens that easier to take everything out rather than
move Quorum Calculation to new place atm:
- new module llmq/options have a code related to various params, command
line options, spork-related etc
- llmq/utils is not included in various files which do not use any
llmq/utils code
- helper `BuildCommitmentHash` goes to llmq/commitment
- helper `BuildSignHash` goes to llmq/signing
- helper `GetLLMQParam` inlined since it's trivial (it has not been
trivial when introduced ages ago)
- removed dependency of `IsQuorumEnabled` on CQuorumManager which means
`quorumManager` deglobalization is done for 90%
## How Has This Been Tested?
- Run unit functional tests
- updated circular dependencies
`test/lint/lint-circular-dependencies.sh`
- check that llmq/utils is not included without needs to calculate
Quorums Members
```
$ grep -r include src/ 2> /dev/null | grep -v .Po: | grep -vE 'llmq/utils.(h|cpp)': | grep llmq/utils
src/evo/mnauth.cpp:#include <llmq/utils.h>
src/evo/deterministicmns.cpp:#include <llmq/utils.h>
src/llmq/quorums.cpp:#include <llmq/utils.h>
src/llmq/blockprocessor.cpp:#include <llmq/utils.h>
src/llmq/commitment.cpp:#include <llmq/utils.h>
src/llmq/debug.cpp:#include <llmq/utils.h>
src/llmq/dkgsessionhandler.cpp:#include <llmq/utils.h>
src/llmq/dkgsession.cpp:#include <llmq/utils.h>
src/llmq/dkgsessionmgr.cpp:#include <llmq/utils.h>
src/rpc/quorums.cpp:#include <llmq/utils.h>
```
## Breaking Changes
N/A
## 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
## Issue being fixed or feature implemented
Some headers include other heavy headers, such as `logging.h`,
`tinyformat.h`, `iostream`. These headers are heavy and increase
compilation time on scale of whole project drastically because can be
used in many other headers.
## What was done?
Moved many heavy includes from headers to cpp files to optimize
compilation time.
In some places added forward declarations if it is reasonable.
As side effect removed 2 circular dependencies:
```
"llmq/debug -> llmq/dkgsessionhandler -> llmq/debug"
"llmq/debug -> llmq/dkgsessionhandler -> llmq/dkgsession -> llmq/debug"
```
## How Has This Been Tested?
Run build 2 times before refactoring and after refactoring: `make clean
&& sleep 10s; time make -j18`
Before refactoring:
```
real 5m37,826s
user 77m12,075s
sys 6m20,547s
real 5m32,626s
user 76m51,143s
sys 6m24,511s
```
After refactoring:
```
real 5m18,509s
user 73m32,133s
sys 6m21,590s
real 5m14,466s
user 73m20,942s
sys 6m17,868s
```
~5% of improvement for compilation time. That's not huge, but that's
worth to get merged
There're several more refactorings TODO but better to do them later by
backports:
- bitcoin/bitcoin#27636
- bitcoin/bitcoin#26286
- bitcoin/bitcoin#27238
- and maybe this one: bitcoin/bitcoin#28200
## Breaking Changes
N/A
## 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
## Issue being fixed or feature implemented
Now that v19 is buried we can enforce basic bls scheme usage in
governance and coinjoin and drop some extra code we used for backwards
compatibility.
## What was done?
pls see individual commits
## How Has This Been Tested?
run tests, sync and mix on testnet
## Breaking Changes
n/a
## 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)_
## Issue being fixed or feature implemented
Use Spans instead of const std::vector<T>&
## What was done?
Replaced with Span
## How Has This Been Tested?
Building, ran a few tests
## Breaking Changes
Should be none, please review potential lifetime issues in bls_worker;
it scares me a bit and I don't understand how we know these won't
dangle.
## 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)_
