f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f Add logging for CValidationInterface events (Jeffrey Czyz)
6edebacb2191373e76d79a4972d6192300976096 Refactor FormatStateMessage for clarity (Jeffrey Czyz)
72f3227c83810936e7a334304e5fd7c6dab8e91b Format CValidationState properly in all cases (Jeffrey Czyz)
428ac70095253225f64462ee15c595644747f376 Add VALIDATION to BCLog::LogFlags (Jeffrey Czyz)
Pull request description:
Add logging of `CValidationInterface` callbacks using a new `VALIDATIONINTERFACE` log flag (see #12994). A separate flag is desirable as the logging can be noisy and thus may need to be disabled without affecting other logging.
This could help debug issues where there may be race conditions at play, such as #12978.
ACKs for top commit:
jnewbery:
ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f
hebasto:
ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f
ariard:
ACK f9abf4a, only changes since 0cadb12 are replacing log indication `VALIDATIONINTERFACE` by `VALIDATION` and avoiding a forward declaration with a new include
ryanofsky:
Code review ACK f9abf4ab6d3d3e4d4b7e90723020b5422a141a6f. Just suggested changes since last review (thanks!)
Tree-SHA512: 3e0f6e2c8951cf46fbad3ff440971d95d526df2a52a2e4d6452a82785c63d53accfdabae66b0b30e2fe0b00737f8d5cb717edbad1460b63acb11a72c8f5d4236
e09c701e0110350f78366fb837308c086b6503c0 scripted-diff: Bump copyright of files changed in 2020 (MarcoFalke)
6cbe6209646db8914b87bf6edbc18c6031a16f1e scripted-diff: Replace CCriticalSection with RecursiveMutex (MarcoFalke)
Pull request description:
`RecursiveMutex` better clarifies that the mutex is recursive, see also the standard library naming: https://en.cppreference.com/w/cpp/thread/recursive_mutex
For that reason, and to avoid different people asking me the same question repeatedly (e.g. https://github.com/bitcoin/bitcoin/pull/15932#pullrequestreview-339175124 ), remove the outdated alias `CCriticalSection` with a scripted-diff
## Issue being fixed or feature implemented
Speed thing up: 8075fc0c61
Unify things: ff1a390224 (and _probably_
fix issues like https://gitlab.com/dashpay/dash/-/jobs/4304343867),
876f5c3a9f,
ed58cdda13
Let tsan tests finish on smaller/slower machines:
ba1e3360f9
## What was done?
pls see individual commits
## How Has This Been Tested?
run tests locally and my in gitlab ci
https://gitlab.com/UdjinM6/dash/-/jobs/4319419014
## 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
- [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 _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
This changes are follow up for backport bitcoin/bitcoin#16060
## What was done?
Buried all hardened dash deployments
## How Has This Been Tested?
Run unit/functional tests.
Run dash with option `-reindex` for both mainnet/testnet - both succeed.
## Breaking Changes
No breaking changes, it should be fully compatible.
## 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
Move `BuildSimplifiedDiff` to the place it's actually used. This also
resolves 3 circular dependencies we have atm.
## What was done?
mostly trivial move-only changes
## How Has This Been Tested?
it compiles and linter is happy locally
## 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
Implementation of Randomness Beacon Part 2.
This PR is the next step of #5262.
Starting from v20 activation fork, members for quorums are sorted using
(if available) the best CL signature found in Coinbase.
If no CL signature is present yet, then the usual way is used (By using
Blockhash instead)
## What was done?
## How Has This Been Tested?
Test `feature_llmq_rotation.py` was updated to cover both rotated and
non-rotated quorums.
2 quorums are mined first to ensure Chainlock are working earlier.
Then dip_24 activation is replaced by v20 activation.
The only direct way to test this change is to make sure that all
expected quorums after v20 activation are properly formed.
Note: A `wait_for_chainlocked_block_all_nodes` is called between every
rotation cycle to ensure that Coinbase will use a different Chainlock
signature.
## Breaking Changes
Yes, quorum members will be calculated differently.
## 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 _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
We had forgotten to harden dip20 and dip24 activation
## What was done?
Hardened dip20 and dip24 activation
## How Has This Been Tested?
Hasn't yet; should do an assumevalid=0 reindex
## Breaking Changes
Hopefully 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
- [ ] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Install of dash_hash will change once
https://github.com/dashpay/dash_hash/pull/17/ is merged
## What was done?
- Changed install instructions to match new install in dash_hash README
- Updated Dockerfile to install correctly
## How Has This Been Tested?
N/A
## 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
- [x] I have made corresponding changes to the documentation
## Issue being fixed or feature implemented
I think the logic in activate_by_name is broken
## What was done?
fix it
## How Has This Been Tested?
run tests
## 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 _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
`feature_pruning.py` is failing atm
## What was done?
pls see individual commits
## How Has This Been Tested?
run `feature_pruning.py` locally
gitian for this branch with `INTEGRATION_TESTS_ARGS` set to `--extended
--exclude feature_dbcrash --timeoutscale=4 --jobs=4`:
https://gitlab.com/dashpay/dash/-/pipelines/852044104
NOTE:
[`linux64_tsan-test`](https://gitlab.com/dashpay/dash/-/jobs/4197383660)
failed because tsan build binaries are super slow and we hit 30 minutes
timeout for 1 single test because of that. This is not an actual test
failure.
## 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 _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
## What was done?
- Bumped version of `CbTx`. Added fields `bestCLHeightDiff`,
`bestCLSignature`
- Miner starting from v20 fork, includes best CL signature in `CbTx` (if
available) or null signature.
- All nodes should verify included CL signature before accepting the
block.
## How Has This Been Tested?
Basically, activated v20 on in the beginning of
`feature_llmq_chainlocks.py`
## Breaking Changes
Yes, new version of CbTx
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] 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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Functional tests in CI and locally often fails without a good reason
(pretty randomly)
## What was done?
It was re-implemented `get_recovered_sig` and updated `create_raw_tx`
for better selection/change output.
## How Has This Been Tested?
Run functional times with bug amount of parrallel jobs:
```
test/functional/test_runner.py -j 20 feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py feature_llmq_is_cl_conflicts.py
```
Without these changes usually 2-3 instance fails.
With these changes all failures happened only for `p2p_addrv2_relay.py`
and `mempool_unbroadcast.py`. Beside feature_llmq_is_conflicts.py
improved stability of `interface_zmq_dash.py` also.
## Breaking Changes
No breaking changes
## 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
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
e78aaf41f43d0e2ad78fa6d8dad61032c8ef73d0 [docs] Add release notes for burying bip 9 soft fork deployments (John Newbery)
8319e738f9f118025b332e4fa804d4c31e4113f4 [tests] Add coverage for the content of getblockchaininfo.softforks (James O'Beirne)
0328dcdcfcb56dc8918697716d7686be048ad0b3 [Consensus] Bury segwit deployment (John Newbery)
1c93b9b31c2ab7358f9d55f52dd46340397c906d [Consensus] Bury CSV deployment height (John Newbery)
3862e473f0cb71a762c0306b171b591341d58142 [rpc] Tidy up reporting of buried and ongoing softforks (John Newbery)
Pull request description:
This hardcodes CSV and segwit activation heights, similar to the BIP 90 buried deployments for BIPs 34, 65 and 66.
CSV and segwit have been active for over 18 months. Hardcoding the activation height is a code simplification, makes it easier to understand segwit activation status, and reduces technical debt.
This was originally attempted by jl2012 in #11398 and again by me in #12360.
ACKs for top commit:
ajtowns:
ACK e78aaf41f43d0e2ad78fa6d8dad61032c8ef73d0 ; checked diff to previous acked commit, checked tests still work
ariard:
ACK e78aaf4, check diff, run the tests again and successfully activated csv/segwit heights on mainnet as expected.
MarcoFalke:
ACK e78aaf41f43d0e2ad78fa6d8dad61032c8ef73d0 (still didn't check if the mainnet block heights are correct, but the code looks good now)
Tree-SHA512: 7e951829106e21a81725f7d3e236eddbb59349189740907bb47e33f5dbf95c43753ac1231f47ae7bee85c8c81b2146afcdfdc11deb1503947f23093a9c399912
fa3c6575cac5e3841797980fe60b8368ae579dba lint: Add false positive to python dead code linter (MarcoFalke)
fa25668e1c8982548f1c6f94780709c625811469 test: Test p2sh-witness and bech32 in wallet_import_rescan (MarcoFalke)
fa79af298917d501cee26370fdf9d44d05133d15 test: Replace fragile "rng" with call to random() (MarcoFalke)
fac3dcf7d052586548f2100a0d576618a85741f9 test: Generate one block for each send in wallet_import_rescan (MarcoFalke)
Pull request description:
This adds test coverage for segwit in the `wallet_import_rescan` test, among other cleanups.
ACKs for top commit:
jnewbery:
ACK fa3c6575cac5e3841797980fe60b8368ae579dba
Tree-SHA512: 877741763c62c1bf9d868864a1e3f0699857e8c028e9fcd65c7eeb73600c22cbe97b7b51093737743d9e87bcb991c1fe1086f673e18765aef0fcfe27951402f0
9c23ebd6b18fb1058a8d3e8aae9e0595d3a57ad5 qa: Fix service flag comparison check in rpc_net test (Luke Dashjr)
Pull request description:
Rebase of #16936
ACKs for top commit:
darosior:
ACK 9c23ebd6b18fb1058a8d3e8aae9e0595d3a57ad5
Tree-SHA512: 74f287740403da1040ab1e235ef6eba4e304f3ee5d57a3b25d1e2e1f2f982d256528d398a4d6cb24ba393798e680a8f46cd7dae54ed84ab2c747e96288f1f884
8925df86c4df16b1070343fef8e4d238f3cc3bd1 doc: update release notes (Jon Atack)
8bb405bbadf11391ccba7b334b4cfe66dc85b390 test: getaddressinfo labels purpose deprecation test (Jon Atack)
60aba1f2f11529add115d963d05599130288ae28 rpc: simplify getaddressinfo labels, deprecate previous behavior (Jon Atack)
7851f14ccf2bcd1e9b2ad48e5e08881be06d9d21 rpc: incorporate review feedback from PR 17283 (Jon Atack)
Pull request description:
This PR builds on #17283 (now merged) and is followed by #17585.
It modifies the value returned by rpc getaddressinfo `labels` to an array of label name strings and deprecates the previous behavior of returning an array of JSON hash structures containing label `name` and address `purpose` key/value pairs.
before
```
"labels": [
{
"name": "DOUBLE SPEND",
"purpose": "receive"
}
```
after
```
"labels": [
"DOUBLE SPEND"
]
```
The deprecated behavior can be re-enabled by starting bitcoind with `-deprecatedrpc=labelspurpose`.
For context, see:
- https://github.com/bitcoin/bitcoin/pull/17283#issuecomment-554458001
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-12-13.html#l-425 (lines 425-427)
- http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-22.html#l-622
Reviewers: This PR may be tested manually by building, then running bitcoind with and without the `-deprecatedrpc=labelspurpose` flag while verifying the rpc getaddressinfo help text and `labels` output.
Next steps: deprecate the rpc getaddressinfo `label` field (EDIT: done in #17585) and add support for multiple labels per address. This PR will unblock those.
ACKs for top commit:
jnewbery:
reACK 8925df8
promag:
Code review ACK 8925df86c4df16b1070343fef8e4d238f3cc3bd1.
meshcollider:
Code review ACK 8925df86c4df16b1070343fef8e4d238f3cc3bd1
Tree-SHA512: c2b717209996da32b6484de7bb8800e7048410f9ce6afdb3e02a6866bd4a8f2c730f905fca27b10b877b91cf407f546e69e8c4feb9cd934325a6c71c166bd438
3bd8db80d8d335ab63ece4f110b0fadd562e80b7 [validation] fix comments in CheckInputScripts() (John Newbery)
6f6465cefcd599c89c00f7b51f42a4b87a5ffb0b scripted-diff: [validation] Rename CheckInputs to CheckInputScripts (John Newbery)
Pull request description:
CheckInputs() used to check no double spends, scripts & sigs and amounts. Since
832e074, the double spend and amount checks
have been moved to CheckTxInputs(), and CheckInputs() now just validates
input scripts. Rename the function to CheckInputScripts().
Also fix incorrect comments.
ACKs for top commit:
MarcoFalke:
re-ACK 3bd8db80d8d335ab63ece4f110b0fadd562e80b7, did the rebase myself, checked the scripted diff 👡
promag:
ACK 3bd8db80d8d335ab63ece4f110b0fadd562e80b7 :trollface:
Tree-SHA512: 7b3f8597d210492798fb784ee8ea47ea6377519111190161c7cc34a967509013f4337304f52e9bedc97b7710de7b0ff8880e08cd7f867754567f82e7b02c794c
e2ff385e138562fb3e1cc63bdd58715a2d8bad98 test: check for invalid `-prune` parameters (Sebastian Falbesoner)
Pull request description:
This small PR adds missing test coverage for invalid `-prune` parameter values / combinations:
77e23ca945/src/init.cpp (L926-L928)77e23ca945/src/init.cpp (L935-L937)77e23ca945/src/init.cpp (L844-L849)
Not sure if the tests fit into `feature_config_args.py` or should rather be moved into `feature_pruning.py`; the latter though seems to be run less often due to being very memory-hungry.
ACKs for top commit:
laanwj:
Code review ACK e2ff385e138562fb3e1cc63bdd58715a2d8bad98
Tree-SHA512: bb0db98090058ecac9f8a01301634e9dba9a65fd56b6a0b770f88da28c4f01e240e22b1225f0d231e28bdd4b5b51bff0e6853cccc46ed0190e91b84f7954a9db
c5bb142817c53c6a217163958b5d511f12171004 test: resolve bug in test/functional/interface_bitcoin_cli.py - Test -getinfo with -rpcwallet=unloaded wallet returns no balances (klementtan)
Pull request description:
I think there is a bug in this test case where the new value of `cli_get_info` is not asserted.
ACKs for top commit:
jonatack:
ACK c5bb142817c53c6a217163958b5d511f12171004
Tree-SHA512: 50c0c2c8fe63c95f951dee892fbacedf92208f47efe5ed481fbb255f15137c799d9200fa3ff31a442df0691248d7ff04d899842722c3032cd7f35553622ba38c
eadd1304c81e0b89178e4cc7630bd31650850c85 tests: Add a test for funding with sufficient preset inputs and subtractFeeFromOutputs (Andrew Chow)
ff330badd45067cb520b1cfa1844f60a4c9f2031 Default to bnb_used = false as there are many cases where BnB is not used (Andrew Chow)
Pull request description:
#17290 introduced a bug where, when we had preset inputs that covered the amount being sent and subtractFeeFrromOutputs was being used, transaction funding would result in a `Fee exceeds maximum configured by -maxtxfee` error. This was happening because we weren't setting `bnb_used = false` when the preset inputs were used as it should have been. This resulted in a too high fee because the change would go to fees accidentally.
Apparently this particular case doesn't have a test, so I've added one as well.
ACKs for top commit:
Sjors:
ACK eadd130. I can't get this new test to fail on macOS (without this PR). It passes whether or not I compile with `--enable-debug`. It does fail on Ubuntu. Yay undefined behavior... Anyway, it's a useful test.
fanquake:
ACK eadd1304c81e0b89178e4cc7630bd31650850c85
instagibbs:
utACK eadd1304c8
Tree-SHA512: 7286c321f78666eea558cc591174630d210263594df41cab1065417510591ee514ade0e1d0cec8af09a785757da68de82592b013e8fe8d4966cec3254368706e
33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151 test: add rpc getaddressinfo labels test coverage (Jon Atack)
0f3539ac6d772fc646b5f184fa1efe77bf632f6a test: add listlabels test in wallet_labels.py (Jon Atack)
1388de83900eaced906d369fe9e8887ae74b2dcf rpc: add getaddressinfo code documentation (Jon Atack)
2ee0cb3330ccf70f0540cb42370796e32eff1569 rpc: update getaddressinfo RPCExamples to bech32 (Jon Atack)
8d1ed0c263f8cdff7189f02040b5d02238d93da0 rpc: clarify label vs labels in getaddressinfo RPCHelpman (Jon Atack)
5a0ed850700dfb19167d40b38f80313bd5e427ca rpc: improve getaddressinfo RPCHelpman content (Jon Atack)
70cda342cd20d0e0cd9f28405457544036968f2d rpc: improve getaddressinfo RPCHelpman formatting (Jon Atack)
Pull request description:
This PR is a continuation of the work in https://github.com/bitcoin/bitcoin/pull/12892.
Main motivations:
- There is currently no test coverage for the getaddressinfo `labels` response. Coverage here is a prerequisite before deprecating the `label` response or adding multiple labels per address.
- `bitcoin-cli help getaddressinfo` returns a few content errors, difficult-to-read formatting, and no explanation why it returns both `label` and `labels` and how they relate, which can be confusing for application developers.
Changes by order of commits:
- [x] improve/fix getaddressinfo RPCHelpman layout formatting
- [x] improve/fix getaddressinfo RPCHelpman content
- [x] clarify the `label` and `labels` fields in getaddressinfo RPCHelpman
- [x] update getaddressinfo RPCExamples addresses to bech32
- [x] add getaddressinfo code docs
- [x] add a `listlabels` test assertion in wallet_labels.py
- [x] add missing getaddressinfo `labels` test coverage and improve the existing `label` tests
Here are gists of the CLI help output:
[`bitcoin-cli help getaddressinfo` before this PR](https://gist.github.com/jonatack/022af5221a85c069780359a22643c810)
[`bitcoin-cli help getaddressinfo` after this PR](https://gist.github.com/jonatack/4ee5f6abc62a3d99269570206a5f90ba)
It seems we ought to begin a deprecation process for the getaddressinfo `label` field? If yes, I have a follow-up ready. _--> EDIT: Deprecation follow-ups #17578 and #17585 now build on this PR._
ACKs for top commit:
fjahr:
Re-ACK 33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151
jnewbery:
ACK 33f5fc32e5bfbe1e89c4d20ce455bcc6dc194151.
Tree-SHA512: a001aa863090ec2566a31059477945b1c303ebeb430b33472f8b150e420fa5742fc33bca9d95571746395b607f43f6078dd5b53e238ac1f3fc648b51c8f79a07
fabd71076cd9493bd2d30a198467f5ea621b27aa ci: Print free disk space (MarcoFalke)
fad9fdbea5dfb19328282afda9588edc6f1d0ddf test: Properly deserialize integers in little-endian (MarcoFalke)
fa94fc10c881e502e6c9a71f3b7719aa955900f9 ci: Run functional tests on s390x (MarcoFalke)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: 98ba77eb56f283131fdaeb393fda86cc308f1bf9781e1e0e5736b8d616528dc8ff2e494d55ba107c138083025c66a59e382fcfa9962d4349a5fd6cbbc52484c3
4671fc3d9e669da8b8781f0cbefee43cb9acd527 Expand on wallet_balance.py comment from https://github.com/bitcoin/bitcoin/pull/16766\#issuecomment-527563982 (Jeremy Rubin)
91f3073f08aff395dd813296bf99fd8ccc81bb27 Update release notes to mention changes to IsTrusted and impact on wallet (Jeremy Rubin)
8f174ef112199aa4e98d756039855cc561687c2e Systematize style of IsTrusted single line if (Jeremy Rubin)
b49dcbedf79613f0e0f61bfd742ed265213ed280 update variable naming conventions for IsTrusted (Jeremy Rubin)
5ffe0d144923f365cb1c2fad181eca15d1668692 Update comment in test/functional/wallet_balance.py (Jeremy Rubin)
a550c58267f50c59c2eea1d46edaa5019a8ad5d8 Update wallet_balance.py test to reflect new behavior (Jeremy Rubin)
5dd7da4ccd1354f09e2d00bab29288db0d5665d0 Reuse trustedParents in looped calls to IsTrusted (Jeremy Rubin)
595f09d6de7f1b94428cdd1310777aa6a4c584e5 Cache tx Trust per-call to avoid DoS (Jeremy Rubin)
dce032ce294fe0d531770f540b1de00dc1d13f4b Make IsTrusted scan parents recursively (Jeremy Rubin)
Pull request description:
This slightly modifies the behavior of IsTrusted to recursively check the parents of a transaction. Otherwise, it's possible that a parent is not IsTrusted but a child is. If a parent is not trusted, then a child should not be either.
This recursive scan can be a little expensive, so ~it might be beneficial to have a way of caching IsTrusted state, but this is a little complex because various conditions can change between calls to IsTrusted (e.g., re-org).~ I added a cache which works per call/across calls, but does not store the results semi-permanently. Which reduces DoS risk of this change. There is no risk of untrusted parents causing a resource exploitation, as we immediately return once that is detected.
This is a change that came up as a bug-fix esque change while working on OP_SECURETHEBAG. You can see the branch where this change is important here: https://github.com/bitcoin/bitcoin/compare/master...JeremyRubin:stb-with-rpc?expand=1. Essentially, without this change, we can be tricked into accepting an OP_SECURETHEBAG output because we don't properly check the parents. As this was a change which, on its own, was not dependent on OP_SECURETHEBAG, I broke it out as I felt the change stands on its own by fixing a long standing wallet bug.
The test wallet_balance.py has been corrected to meet the new behavior. The below comment, reproduced, explains what the issue is and the edge cases that can arise before this change.
# Before `test_balance()`, we have had two nodes with a balance of 50
# each and then we:
#
# 1) Sent 40 from node A to node B with fee 0.01
# 2) Sent 60 from node B to node A with fee 0.01
#
# Then we check the balances:
#
# 1) As is
# 2) With transaction 2 from above with 2x the fee
#
# Prior to #16766, in this situation, the node would immediately report
# a balance of 30 on node B as unconfirmed and trusted.
#
# After #16766, we show that balance as unconfirmed.
#
# The balance is indeed "trusted" and "confirmed" insofar as removing
# the mempool transactions would return at least that much money. But
# the algorithm after #16766 marks it as unconfirmed because the 'taint'
# tracking of transaction trust for summing balances doesn't consider
# which inputs belong to a user. In this case, the change output in
# question could be "destroyed" by replace the 1st transaction above.
#
# The post #16766 behavior is correct; we shouldn't be treating those
# funds as confirmed. If you want to rely on that specific UTXO existing
# which has given you that balance, you cannot, as a third party
# spending the other input would destroy that unconfirmed.
#
# For example, if the test transactions were:
#
# 1) Sent 40 from node A to node B with fee 0.01
# 2) Sent 10 from node B to node A with fee 0.01
#
# Then our node would report a confirmed balance of 40 + 50 - 10 = 80
# BTC, which is more than would be available if transaction 1 were
# replaced.
The release notes have been updated to note the new behavior.
ACKs for top commit:
ariard:
Code Review ACK 4671fc3, maybe extend DoS protection in a follow-up PR.
fjahr:
Code review ACK 4671fc3d9e669da8b8781f0cbefee43cb9acd527
ryanofsky:
Code review ACK 4671fc3d9e669da8b8781f0cbefee43cb9acd527. Changes since last review: 2 new commits adding suggested release note and python test comment, also a clean rebase with no changes to the earlier commits. The PR description is more comprehensive now, too. Looks good!
promag:
Code review ACK 4671fc3d9e669da8b8781f0cbefee43cb9acd527.
Tree-SHA512: 6b183ff425304fef49724290053514cb2770f4a2350dcb83660ef24af5c54f7c4c2c345b0f62bba60eb2d2f70625ee61a7fab76a7f491bb5a84be5c4cc86b92f
bd52684508ca2964e6a3af503d21ff99675380c7 test: rest /tx with an invalid/unknown txid (brunoerg)
Pull request description:
This PR adds test coverage to the endpoint `/tx` (rest) passing an invalid and an unknown txid to test its return.
Invalid -> should return status code 400 (bad request)
Unknown -> should return status code 404 (not found)
ACKs for top commit:
kallewoof:
ACK bd52684508ca2964e6a3af503d21ff99675380c7
Tree-SHA512: a7fbb63f30d06fc0855133a36e8317c7930ba13aa2b4a2dd1fc35079d59eacace72e1ffe7ae1b3e067066fe51792415940d72d923e83a659a0d5965e4110b32a
0754e9c01bd3d57aa241e313ba34c18c4897ba98 test: run feature_pruning.py without wallet compiled (Sebastian Falbesoner)
Pull request description:
Only one small part of the pruning test (sub-test `wallet_test`) is wallet-related, hence we can run all other parts without wallet compiled.
ACKs for top commit:
MarcoFalke:
cr ACK 0754e9c01bd3d57aa241e313ba34c18c4897ba98
Tree-SHA512: 856856903d21d64953ed0102cc2a96f55975c4b7d8e93e57b82c266024967160df64df2b6068be089efc05e883e8d6d12e7327053420d4c640b9d8cc5bcb1c58
fa165e954579436fe4b636e4222d8ce0c1269786 Replace stoul with ToIntegral in dbwrapper (MarcoFalke)
Pull request description:
The string is created with `%llu`. See: 7fcf53f7b4/src/leveldb/db/db_impl.cc (L1436-L1437)
So it seems odd to silently accept when parsing: whitespace, a sign character, trailing chars, overflow, ....
Fix that by using the stricter ToIntegral.
ACKs for top commit:
laanwj:
Code review ACK fa165e954579436fe4b636e4222d8ce0c1269786
practicalswift:
cr ACK fa165e954579436fe4b636e4222d8ce0c1269786
theStack:
Code-review ACK fa165e954579436fe4b636e4222d8ce0c1269786
Tree-SHA512: b87f01431ca0b971ff84610022da8679d3c33470b88cfc3f4a337e6e176a0455715588aefd40e8e2bbe7459d902dc89d7bfe34e7fd66755f631cc18dc039fa2f
3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5bb1e37b2e6ab2c52791ac05d9271238 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b944f6ad51b3c895837729d3aa56eea [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b3174d19a6d8691ae07e92b32fdfaef11 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e49246822421a7bcc720491427e1dba8a3 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed9afbe5a96caa5f0f4cbec730d27460 [validation] Add CValidationState subclasses (John Newbery)
Pull request description:
Carries out some remaining tidy-ups remaining after PR 15141:
- split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
- various minor code style tidy-ups to the ValidationState class
- remove the useless `ret` parameter from `ValidationState::Invalid()`
- remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
- remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.
Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:
Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.
```sh
git checkout <CommitHash>
git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
git diff HEAD^
```
After that it's possible to easily see the mechanical changes with:
```sh
git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
```
ACKs for top commit:
laanwj:
ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf
amitiuttarwar:
code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally.
fjahr:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review.
ryanofsky:
Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review.
Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36
9075d13153ce06cd59a45644831ecc43126e1e82 [docs] Add release notes for removal of REJECT reasons (John Newbery)
04a2f326ec0f06fb4fce1c4f93500752f05dede8 [validation] Fix REJECT message comments (John Newbery)
e9d5a59e34ff2d538d8f5315efd9908bf24d0fdc [validation] Remove REJECT code from CValidationState (John Newbery)
0053e16714323c1694c834fdca74f064a1a33529 [logging] Don't log REJECT code when transaction is rejected (John Newbery)
a1a07cfe99fc8cee30ba5976dc36b47b1f6532ab [validation] Fix peer punishment for bad blocks (John Newbery)
Pull request description:
We no longer send BIP 61 REJECT messages, so there's no need to set
a REJECT code in the CValidationState object.
Note that there is a minor bug fix in p2p behaviour here. Because the
call to `MaybePunishNode()` in `PeerLogicValidation::BlockChecked()` only
previously happened if the REJECT code was > 0 and < `REJECT_INTERNAL`,
then there are cases were `MaybePunishNode()` can get called where it
wasn't previously:
- when `AcceptBlockHeader()` fails with `CACHED_INVALID`.
- when `AcceptBlockHeader()` fails with `BLOCK_MISSING_PREV`.
Note that `BlockChecked()` cannot fail with an 'internal' reject code. The
only internal reject code was `REJECT_HIGHFEE`, which was only set in
ATMP.
This reverts a minor bug introduced in 5d08c9c579.
ACKs for top commit:
ariard:
ACK 9075d13, changes since last reviewed are splitting them in separate commits to ease understanding and fix nits
fjahr:
ACK 9075d13153ce06cd59a45644831ecc43126e1e82, confirmed diff to last review was fixing nits in docs/comments.
ryanofsky:
Code review ACK 9075d13153ce06cd59a45644831ecc43126e1e82. Only changes since last review are splitting the main commit and updating comments
Tree-SHA512: 58e8a1a4d4e6f156da5d29fb6ad6a62fc9c594bbfc6432b3252e962d0e9e10149bf3035185dc5320c46c09f3e49662bc2973ec759679c0f3412232087cb8a3a7
ea4cc3a7b36a9c77dbf0aff439da3ef0ea58e6e4 Truly decouple wallet from chainparams for -fallbackfee (Jorge Timón)
Pull request description:
Before it was 0 by default for main and 20000 for test and regtest.
Now it is 0 by default for all chains, thus there's no need to call Params().
Also now the default for main is properly documented.
Suggestion for release notes:
-fallbackfee was 0 (disabled) by default for the main chain, but 20000 by default for the test chains. Now it is 0 by default for all chains. Testnet and regtest users will have to add fallbackfee=20000 to their configuration if they weren't setting it and they want it to keep working like before.
Should I propose them to the wiki for the release notes or only after merge?
For more context, see https://github.com/bitcoin/bitcoin/pull/16402#issuecomment-515701042
ACKs for top commit:
MarcoFalke:
ACK ea4cc3a7b36a9c77dbf0aff439da3ef0ea58e6e4
Tree-SHA512: fdfaba5d813da4221e405e0988bef44f3856d10f897a94f9614386d14b7716f4326ab8a6646e26d41ef3f4fa61b936191e216b1b605e9ab0520b0657fc162e6c
----
Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>
0ccb3addf68067200892963521a92713c4667a63 tests: Remove no longer needed UBSan suppression (float-divide-by-zero in validation.cpp) (practicalswift)
Pull request description:
Remove no longer needed UBSan suppression.
The float divide-by-zero in `validation.cpp` was fixed by instagibbs in ec30a79f1c430cc7fbda37e5d747b0b31b262fa5 (#15283).
ACKs for top commit:
MarcoFalke:
ACK 0ccb3addf68067200892963521a92713c4667a63
Tree-SHA512: 89a4f4b7371fa5725d9f801cee7ebbd17523f66017c9acfa813657dcb8d837f42209eff44ce9e5d48296a630bab9599d75f10024a0c7da7defb228f4eae3392a
5e146022daa4336de94447e5b8e5418296286927 wallet: fix scanning progress calculation for single block range (Sebastian Falbesoner)
Pull request description:
If the blockchain is rescanned for a single block (i.e. start and stop hashes are equal, and with that also the estimated start/stop verification progress values) the progress calculation could lead to a NaN value caused by a division by zero (0.0/0.0), resulting in an invalid JSON result for the `getwalletinfo` RPC. This PR fixes this behaviour by setting the progress to zero in that special case. Fixes#20297.
The behaviour can easily be reproduced by continuously running single block rescans in an endless loop, e.g. via
```bash
#!/bin/bash
while true
do
bitcoin-cli rescanblockchain $(bitcoin-cli getblockcount)
done
```
and at the same time perform some `getwalletinfo` RPCs.
On the master branch, this leads to frequent invalid responses (tested on mainchain):
```
$ bitcoin-cli getwalletinfo
error: couldn't parse reply from server
$ curl --user `cat ~/.bitcoin/.cookie` --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getwalletinfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
{"result":{"walletname":"","walletversion":169900,"format":"bdb","balance":0.00000000,"unconfirmed_balance":0.00000000,"immature_balance":0.00000000,"txcount":0,"keypoololdest":1603677276,"keypoolsize":1000,"hdseedid":"3196e33ecb47c7130e6ca60f2f895f9259860dca","keypoolsize_hd_internal":1000,"paytxfee":0.00000000,"private_keys_enabled":true,"avoid_reuse":false,"scanning":{"duration":0,"progress":},"descriptors":false},"error":null,"id":"curltest"}
```
(note that missing value for "progress" in the JSON result).
On the PR branch, the behaviour doesn't occur anymore.
ACKs for top commit:
MarcoFalke:
review ACK 5e146022daa4336de94447e5b8e5418296286927
promag:
Core review ACK 5e146022daa4336de94447e5b8e5418296286927.
Tree-SHA512: f0e6aad5a6cd08b36c5fe820fff0ef26663229b39169a4dbe757f3c795a41cf5c69c9dc90efe7515675ae1059307f8971123781a0514d10704123a6f28b125ab
## Issue being fixed or feature implemented
Upgraded version of cppcheck
## What was done?
## How Has This Been Tested?
Ran cppcheck
## 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)_
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] ~~I have commented my code, particularly in hard-to-understand
areas~~ N/A
- [ ] ~~I have added or updated relevant unit/integration/functional/e2e
tests~~ N/A
- [ ] ~~I have made corresponding changes to the documentation~~ N/A
- [ ] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
9c891b64ffd14bc8216dbd5eb60816043af265b6 net: initialize nMessageSize to max uint32_t instead of -1 (eugene)
Pull request description:
nMessageSize is uint32_t and is set to -1. This will warn with `-fsanitize=implicit-integer-sign-change` when V1TransportDeserializer calls into the ctor. This pull initializes nMessageSize to `numeric_limits<uint32_t>::max()` instead and removes the ubsan suppression.
ACKs for top commit:
laanwj:
Code review ACK 9c891b64ffd14bc8216dbd5eb60816043af265b6
promag:
Code review ACK 9c891b64ffd14bc8216dbd5eb60816043af265b6.
Tree-SHA512: f05173d9553a01d207a5a7f8ff113d9e11354c50b494a67d44d3931c151581599a9da4e28f40edd113f4698ea9115e6092b2a5b7329c841426726772076c1493
