## 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
`cs_map_quorums` was introduced to protect `mapQuorumsCache` only. We
shouldn't hold it for too long or require it to be held in
`BuildQuorumFromCommitment`.
## What was done?
limit the scope of `cs_map_quorums`
## How Has This Been Tested?
build and run tests locally and in gitlab ci
## 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
pls see individual commits
fixes an issue (reported by @strophy recently) where mixing wouldn't
start in a fresh new wallet
not 100% sure but
[99867eb](99867eb769)
might also fix#5350 reported by @splawik21 so this could also be a v19
backport candidate
## What was done?
## How Has This Been Tested?
mixing on testnet
## 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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
## 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
gobject sync is broken after #5322
## What was done?
implement proper serialization
## How Has This Been Tested?
run dash-qt/dashd on testnet/mainnet
## Breaking Changes
n/a, fixes breaking changes introduced earlier
## 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)_
Invalid number of minimum members in comments for LLMQ_25_67
## Issue being fixed or feature implemented
Invalid number of minimum members in comments for LLMQ_25_67
## What was done?
- Replaced `67` with `17`
## How Has This Been Tested?
None
## Breaking Changes
None
## 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
- [x] 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
we failed to backport 13216 correctly in #4359
noticed this while reviewing/testing #5255
## What was done?
fix it
## How Has This Been Tested?
run qt with `-resetguisetting` and check info with and without the patch
on testnet for example (or tweak regtest params and test there)
## 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)_
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
4f7127d1e3a51f0f55d42a08439c516dcc8d1a26 gui: Make Intro consistent with prune checkbox (Hennadii Stepanov)
4824a7d36cf47e766865e0fefe952ec860eb82dd gui: Add Intro::UpdateFreeSpaceLabel() (Hennadii Stepanov)
daa3f3fa9071a229275dd6a1b8445237ddc3fa97 refactor: Add Intro::UpdatePruneLabels() (Hennadii Stepanov)
e4caa82a03df5c6a6d5d29f34ab006d732c6dac1 refactor: Replace static variable with data member (Hennadii Stepanov)
2bede28cd9ec638d8bb32c187ccf12d89345218e util: Add PruneGBtoMiB() function (Hennadii Stepanov)
e35e4b2ba052c9a533626286026dbe0a2d546c5b util: Add PruneMiBtoGB() function (Hennadii Stepanov)
Pull request description:
On master (a6f6333ba253cda83221ee529810cacf930e413f) and on 0.19.0.1 the intro dialog with prune enabled (checkbox "Discard blocks..." is checked) provides a user with wrong info about the required disk space:
Also the paragraph "If you have chosen to limit..." is missed.
---
With this PR when prune checkbox is toggled, the related text labels and the amount of required space shown are updated (previously they were only updated when the data directory was updated):
---
This PR is an alternative to #17035.
**ryanofsky**'s [suggestion](https://github.com/bitcoin/bitcoin/pull/17035#discussion_r337594268) also has been implemented.
ACKs for top commit:
emilengler:
ACK 4f7127d1e3a51f0f55d42a08439c516dcc8d1a26
Sjors:
tACK 4f7127d1e3a51f0f55d42a08439c516dcc8d1a26
ryanofsky:
Code review ACK 4f7127d1e3a51f0f55d42a08439c516dcc8d1a26. It seems like there are a few visible changes here:
jonasschnelli:
utACK 4f7127d1e3a51f0f55d42a08439c516dcc8d1a26
Tree-SHA512: fa0bbdcfafde97d7906cda066cbd4608b936a71cae1b4cda3ee3aa2eed3a9795f279f14c6b1b4997278e094db891c7d3bb695368ba0882347aa42165a86e5172
af112ab62895b145660f4cd7ff842e9cfea2a530 qt: Rename SetPrune() to InitializePruneSetting() (Hennadii Stepanov)
b0bfbe50282877a1eee87118902901a280d6656d refactor: Drop `bool force' parameter (Hennadii Stepanov)
68c9bbe9bc91f882404556998666b1b5acea60e4 qt: Force set nPruneSize in QSettings after intro (Hennadii Stepanov)
a82bd8fa5708c16d1db3edc4e82d70788eb5af19 util: Replace magics with DEFAULT_PRUNE_TARGET_GB (Hennadii Stepanov)
Pull request description:
On master (5622d8f3156a293e61d0964c33d4b21d8c9fd5e0), having `QSettings` set already
```
$ grep nPruneSize ~/.config/Bitcoin/Bitcoin-Qt-testnet.conf
nPruneSize=6
```
enabling prune option in the intro dialog
```
$ ./src/qt/bitcoin-qt -choosedatadir -testnet
```
has no effect:
```
$ grep Prune ~/.bitcoin/testnet3/debug.log
2019-12-08T10:04:41Z Prune configured to target 5722 MiB on disk for block and undo files.
```
---
With this PR:
```
$ grep Prune ~/.bitcoin/testnet3/debug.log
2019-12-08T10:20:35Z Prune configured to target 1907 MiB on disk for block and undo files.
```
This PR has been split of #17453 (the first two commits) as it fixes an orthogonal bug.
Refs:
- https://github.com/bitcoin/bitcoin/pull/17453#discussion_r345424240
- https://github.com/bitcoin/bitcoin/pull/17453#discussion_r350960201
ACKs for top commit:
Sjors:
Code review re-ACK af112ab62895b145660f4cd7ff842e9cfea2a530
ryanofsky:
Code review ACK af112ab62895b145660f4cd7ff842e9cfea2a530. Just suggested changes since last review (thanks!)
promag:
Tested ACK af112ab62895b145660f4cd7ff842e9cfea2a530. Latest suggestions and changes look good to me.
Tree-SHA512: 8ddad34b30dcc2cdcad6678ba8a0b36fa176e4e3465862ef6eee9be0f98d8146705138c9c7995dd8c0990af41078ca743fef1a90ed9240081f052f32ddec72b9
9924bce317b96ab0c57efb99330abd11b6f16b9a [gui] intro: enable pruning by default unless disk is big (Sjors Provoost)
c8de347a9d6c88fe67d77aba6fcce1b7fd66791c [gui] intro: add prune preference (Sjors Provoost)
1bbc49d2078ee53488e214d00eb47462687b05c5 [gui] intro: inform caller if intro was shown (Sjors Provoost)
1957103786f97135f35ababc97efa1b481865eb0 [gui] add explicit prune setter (Sjors Provoost)
1bccf6a52d7fc08d8f605cfb2edc3277ec299c72 [node] add forceSetArg to interface (Sjors Provoost)
Pull request description:
This adds a checkbox to the intro screen to enable pruning from the get go.
If the user has plenty of space, it's unchecked by default:
<img width="671" alt="big" src="https://user-images.githubusercontent.com/10217/63641289-10339000-c6ac-11e9-98d7-caf64dff0da6.png">
If the user has insufficient space it's checked by default:
<img width="897" alt="low" src="https://user-images.githubusercontent.com/10217/63641276-d4002f80-c6ab-11e9-9f5b-a53472f814ff.png">
When the user has barely enough space and is likely to need pruning in the near future, this is shown in yellow and we also check the prune box:
<img width="662" alt="medium" src="https://user-images.githubusercontent.com/10217/63641294-1c1f5200-c6ac-11e9-8ecb-6b69e42b1ece.png">
The cut-off for this 10 GB above `m_assumed_blockchain_size` (`=240` in `chainparams.cpp`).
If the user launches the first time with `-prune=...` then we disable the check box and display the correct size (rounded to GB):
<img width="658" alt="Schermafbeelding 2019-08-24 om 20 23 14" src="https://user-images.githubusercontent.com/10217/63641351-09594d00-c6ad-11e9-94fe-fe5ed562e109.png">
The 2 GB default matches the settings default. The user can't change it in the intro screen, but can change it later. I'm tempted to increase that default to 10 GB, and then have the intro screen reduce it if space is really tight.
Tips for testing:
* move your existing data dir elsewhere
* wipe data dir at every restart (behavior is different if it exists)
* launch with `bitcoin-qt -resetguisettings -lang=en` (there's some space issues in different languages)
* fake your free space by changing `intro.cpp` line 90: `freeBytesAvailable = 5000000000; // 5 GB`
* try both testnet and mainnet, because settings are seperate. In particular note how step 7 in `GuiMain` switches where `QTSettings settings` points to; this had me thoroughly confused on testnet, because I was setting them too early.
ACKs for top commit:
jonasschnelli:
Tested ACK 9924bce317b96ab0c57efb99330abd11b6f16b9a
ryanofsky:
utACK 9924bce317b96ab0c57efb99330abd11b6f16b9a. The changes are very logical, and implement the feature in a clean that way that doesn't add a lot of complication and shouldn't interfere with future improvements. I looked at Luke's branch too, and I think there's also a lot of great stuff there that seems fully compatible with this change.
Tree-SHA512: 9523961451c53aebd347716976bc3a4a398f989dc21e9bbbd357060bd11a8f46c435f068bd421bb31ccb08e55445ef67bc347d8d19a4fb8fde9d6d3f9a3bcbb0
3bf9d8cac09fc88727ef2f2a2bea33b90b625e50 Testchains: Qt: Simplify network/chain styles (Jorge Timón)
052c54ecb02695e5d2694e8e0cbf5ccc89de86e8 Testchains: Generic selection with -chain=<str> in addition of -testnet and -regtest (Jorge Timón)
Pull request description:
Separated from #8994 as suggested by MarcoFalke and Sjors in https://github.com/bitcoin/bitcoin/pull/8994#issuecomment-522555390
You can't really test the qt changes on their own, so to test them, use #8994 .
ACKs for top commit:
MarcoFalke:
ACK 3bf9d8cac09fc88727ef2f2a2bea33b90b625e50
Tree-SHA512: 5b5e6083ebc0a44505a507fac633e7af18037c85e5e73f5d1e6f7e730575d3297ba8a31d1c2441df623b273f061c32d8fa324f4aa6bead01d23e88582029b568
## Issue being fixed or feature implemented
The benefits of using custom struct `maybe_error` is less significant
since the interface `TxValidationState` became simpler after
refactorings from bitcoin#15921 and bitcoin#17004
The refactoring of `maybe_error` as a class result from PR #5109 is
still useful but not for case of TxValidationState.
## What was done?
Unified using TxValidationState in dash's related code.
## How Has This Been Tested?
Run unit/functional tests
## Breaking Changes
No breaking changes, logic are same.
## 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
78e283e656bf1643944ffdb76185f3468eb25895 [test] move wallet helper functions into test library (Martin Zumsande)
f613e5dfdafe708f63ebb5193c44e2bc770c6651 [test] move mining helper functions into test library (Martin Zumsande)
2cb4e8bdc7ef75ae8d95c246af1e8e1f9c7045bd [test] move string helper functions into test library (Martin Zumsande)
Pull request description:
This disbands `test/util.h` and `test/util.cpp` and moves the content into the test utility library recently created in #17542, so that all test utility functions are in one place.
The content of the original files are split into three modules:
1) string helper functions go to `test/util/str`
2) mining helper functions go to the newly created `test/util/mining`
3) wallet helper functions go to the newly created `test/util/wallet`
ACKs for top commit:
MarcoFalke:
ACK 78e283e656bf1643944ffdb76185f3468eb25895 🔧
Tree-SHA512: f182a61e86e76c32bcb84e37f44904d3a4a9c5a321f7a8efdda5368a6623cb8b5a5384ec4f96e67f0357b0c22099f6e3ecd0ac4cb467e3fa3f3128f8d36edfb8
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
4bdd68f301a9cee3360deafc7531c638e923226b Add missing typeinfo includes (Wladimir J. van der Laan)
4d88c3dcb61e7c075ed3dd442044e0eff4e3c8de net: Log to net category for exceptions in ProcessMessages (Wladimir J. van der Laan)
Pull request description:
Remove the forest of special exceptions based on string matching, and simply log a short message to the NET logging category when an exception happens during packet processing. It is not good to panick end users with verbose errors (let alone writing to stderr) when any peer can generate them.
ACKs for top commit:
MarcoFalke:
re-ACK 4bdd68f301a9cee3360deafc7531c638e923226b (only change is adding includes) 🕕
promag:
ACK 4bdd68f301a9cee3360deafc7531c638e923226b, could squash.
Tree-SHA512: a005591a3202b005c75e01dfa54249db3992e2f9eefa8b3d9d435acf66130417716ed926ce4e045179cf43788f1abc7362d999750681a9c80b318373d611c366
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
e9fd366044e271632dc0e4f96e1c14f8e87213ae refactor: Remove null setting check in GetSetting() (Russell Yanofsky)
cba2710220d76bbe790b04088839cbbd410436de scripted-diff: Remove unused ArgsManager type flags in tests (Russell Yanofsky)
425bb307252cf4dec9b3ef6426e6548b2be7a303 refactor: Add util_CheckValue test (Russell Yanofsky)
0fa54358b06b58f4d17073bcc8a959eb9498aadc refactor: Add ArgsManager::GetSettingsList method (Russell Yanofsky)
3e185522ace1678e0a25b9cf8a5553a4bc279bea refactor: Get rid of ArgsManagerHelper class (Russell Yanofsky)
dc0f1480746b34aa3ca2d9c0f1ec764083026b40 refactor: Replace FlagsOfKnownArg with GetArgFlags (Russell Yanofsky)
57e8b7a7273567aa4a4aee87cce18e9bff8f3196 refactor: Clean up includeconf comments (Russell Yanofsky)
3f7dc9b808316c1e5d677af8d9a99112568c8ccb refactor: Clean up long lines in settings code (Russell Yanofsky)
Pull request description:
This PR doesn't change behavior. It just implements some suggestions from #15934 and #16545 and few other small cleanups.
ACKs for top commit:
jnewbery:
Code review ACK e9fd366044e271632dc0e4f96e1c14f8e87213ae
MarcoFalke:
ACK e9fd366044 🚟
Tree-SHA512: 6e100d92c72f72bc39567187ab97a3547b3c06e5fcf1a1b74023358b8bca552124ca6a53c0ab53179b7f1329c03d9a73faaef6d73d2cd1a2321568a0286525e2
## Issue being fixed or feature implemented
I was trying to look at the members inside of CDeterministicMN and
overlooked most of them initially since they're not at the top
## What was done?
Moved the members up
## 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)_
## Issue being fixed or feature implemented
## What was done?
Increased v20 deployment window size in order to delay v20 activation.
Only for regtest
## How Has This Been Tested?
## 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
- [ ] 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
This refactoring is a follow-up changes to backport bitcoin#17164 (PR
#5314)
These changes are reduce difference in implementation for our code and
bitcoin's
## What was done?
Removed a flag m_block_relay_peer. Instead I call IsAddrRelayPeer() that
has same information now.
It changes logic introduced in #4888 due to dash-specific code.
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
No 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
## Issue being fixed or feature implemented
## What was done?
Feature requested by @QuantumExplorer and @iammadab.
This PR introduces `protx listdiff`: a more rich alternative of `protx
diff` RPC.
Currently, `protx diff` is returning data only required from SPV for SML
Coinbase MerkleMNListRoot calculation.
Platform team needed a similar RPC returning all the MNs data in order
to calculate the identities.
## How Has This Been Tested?
## 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
- [ ] 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
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
356988e200b1debaa80d210d502d2d085c72dc64 util: make EncodeBase58Check consume Spans (Sebastian Falbesoner)
f0fce0675d56b2226a993253731690ca864066c8 util: make EncodeBase58 consume Spans (Sebastian Falbesoner)
Pull request description:
This PR improves the interfaces for the functions `EncodeBase58{Check}` by using Spans, in a similar fashion to e.g. PRs #19660, #19687. Note that on the master branch there are currently two versions of `EncodeBase58`: one that takes two pointers (marking begin and end) and another one that takes a `std::vector<unsigned char>` const-ref. The PR branch only leaves one generic Span-interface, both simplifying the interface and allowing more generic containers to be passed. The same is done for `EncodeBase58Check`, where only one interface existed but it's more generic now (e.g. a std::array can be directly passed, as done in the benchmarks).
ACKs for top commit:
laanwj:
Code review ACK 356988e200b1debaa80d210d502d2d085c72dc64
Tree-SHA512: 47cfccdd7f3a2d4694bb8785e6e5fd756daee04ce1652ee59a7822e7e833b4a441ae9362b9bd67ea020d2b5b7d927629c9addb6abaa9881d8564fd3b1257f512
fac9fe5d051264fcd16e8e36d30f28c05c999837 Fix unintended unsigned integer overflow in strencodings (MarcoFalke)
Pull request description:
This fixes two issues for strings that start with a colon and only have one colon:
* `fMultiColon` is incorrectly set to `true`
* There is an unsigned integer overflow `colon - 1` (`0 - 1`)
Neither issue matters, as the result is discarded. Though, it makes sense to still fix the issue for clarity and to avoid sanitizer issues in the function.
ACKs for top commit:
laanwj:
Code review ACK fac9fe5d051264fcd16e8e36d30f28c05c999837
shaavan:
Code Review ACK fac9fe5d051264fcd16e8e36d30f28c05c999837
Tree-SHA512: e71c21a0b617abf241e561ce6b90b963e2d5e2f77bd9547ce47209a1a94b454384391f86ef5d35fedd4f4df19add3896bb3d61fed396ebba8e864e3eeb75ed59
e952d7557eaf2610e302e9d70381ef057d07f6bf netinfo: clarify client and server versions in header (Jon Atack)
Pull request description:
Clarify in -netinfo output that both the client and the server versions are provided.
before
```
Bitcoin Core v22.0.0rc3 - 70016/Satoshi:22.99.0/
```
after
```
Bitcoin Core client v22.0.0rc3 - server 70016/Satoshi:22.99.0/
```
Closes#22873.
ACKs for top commit:
benthecarman:
utACK e952d7557eaf2610e302e9d70381ef057d07f6bf
prayank23:
ACK e952d7557e
Zero-1729:
tACK e952d7557eaf2610e302e9d70381ef057d07f6bf
Tree-SHA512: 3e817892d398aabacb1401fd5b1816c4d4f563b4f8cf1096bdb8b53f7c4ef82d4caee09f5c7724f1fe292f837434a332acefba735152ed24a238bb6f006df909
cdd51e8ee156f3bb3135be8aa51530a53734153e torcontrol: Resolve Tor control plane address (Adrian-Stefan Mares)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/22236
This PR forces the Tor control plane address to be resolved before a connection attempt is made, similar to how the `-proxy` / `-onion` address is resolved.
The use case for this change is that the control plane may not have a stable address - in a containerized environment perhaps.
ACKs for top commit:
jonatack:
ACK cdd51e8ee156f3bb3135be8aa51530a53734153e tested various configurations on signet with this branch versus master
laanwj:
LGTM ACK cdd51e8ee156f3bb3135be8aa51530a53734153e
theStack:
ACK cdd51e8ee156f3bb3135be8aa51530a53734153e 🪐
prayank23:
ACK cdd51e8ee1
Tree-SHA512: 5335cfcb89089a2acd6d02b88c2022dec60bb74388a99187c901c1c35d32896814d5f81df55c053953276c51fcec263c6ddadd068316f8e428b841bd599fc21e
ee11a412a537f62aa46e8862678ce2069a2df5b7 Avoid signed integer overflow when loading a mempool.dat file with a malformed time field (practicalswift)
Pull request description:
Avoid signed integer overflow when loading a `mempool.dat` file with a malformed time field.
Avoid the following signed integer overflow:
```
$ xxd -p -r > mempool.dat-crash-1 <<EOF
0100000000000000000000000004000000000000000000000000ffffffff
ffffff7f00000000000000000000000000
EOF
$ cp mempool.dat-crash-1 ~/.bitcoin/regtest/mempool.dat
$ UBSAN_OPTIONS="print_stacktrace=1:halt_on_error=1:report_error_type=1" src/bitcoind -regtest
validation.cpp:5079:23: runtime error: signed integer overflow: 9223372036854775807 + 1209600 cannot be represented in type 'long'
#0 0x5618d335197f in LoadMempool(CTxMemPool&) src/validation.cpp:5079:23
#1 0x5618d3350df3 in CChainState::LoadMempool(ArgsManager const&) src/validation.cpp:4217:9
#2 0x5618d2b9345f in ThreadImport(ChainstateManager&, std::vector<boost::filesystem::path, std::allocator<boost::filesystem::path> >, ArgsManager const&) src/init.cpp:762:33
#3 0x5618d2b92162 in AppInitMain(util::Ref const&, NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_14::operator()() const src/init.cpp:1881:9
```
This PR was broken out from PR #20089. Hopefully this PR is trivial to review.
Fixes a subset of #19278.
ACKs for top commit:
MarcoFalke:
review ACK ee11a412a537f62aa46e8862678ce2069a2df5b7
Crypt-iQ:
crACK ee11a412a537f62aa46e8862678ce2069a2df5b7
Tree-SHA512: 227ab95cd7d22f62f3191693b455eacfa8e36534961bee12c622fc9090957cfb29992eabafa74d806a336e03385aa8f98b7ce734f04b0b400e33aa187d353337
## Issue being fixed or feature implemented
Avoids magic numbers
## What was done?
converted to std::optional
## How Has This Been Tested?
make check
## Breaking Changes
none
## 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
- [x] 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: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
b6d2183858975abc961207c125c15791e531edcc Minor refactoring to remove implied m_addr_relay_peer. (User)
a552e8477c5bcd22a5457f4f73a2fd6db8acd2c2 added asserts to check m_addr_known when it's used (User)
090b75c14be6b9ba2efe38a17d141c6e6af575cb p2p: Avoid allocating memory for addrKnown where we don't need it (User)
Pull request description:
We should allocate memory for addrKnown filter only for those peers which are expected to participate in address relay.
Currently, we do it for all peers (including SPV and block-relay-only), which results in extra RAM where it's not needed.
Upd:
In future, we would still allow SPVs to ask for addrs, so allocation still will be done by default.
However, they will be able to opt-out via [this proposal](https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2019-October/017428.html) and then we could save some more memory.
This PR still saves memory for block-relay-only peers immediately after merging.
Top commit has no ACKs.
Tree-SHA512: e84d93b2615556d466f5ca0e543580fde763911a3bfea3127c493ddfaba8f05c8605cb94ff795d165af542b594400995a2c51338185c298581408687e7812463
4a96e459d733f1b6427221aaa1874ea00f79988a [gui] send: show watch-only balance in send screen (Sjors Provoost)
2689c8fd7159f47248c5fc365463be8b0e8b039c [test] qt: add send screen balance test (Sjors Provoost)
Pull request description:
Now that we can create a PSBT from a watch-only wallet (#16944), we should also display the watch-only balance on the send screen.
Before:
<img width="1008" alt="before" src="https://user-images.githubusercontent.com/10217/69533384-030e9180-0f78-11ea-9748-c32c957e822e.png">
After:
<img width="1009" alt="Schermafbeelding 2019-11-26 om 11 44 17" src="https://user-images.githubusercontent.com/10217/69622879-19811f80-1042-11ea-8279-091012f39b38.png">
I added a test to check the balance on the send screen, but it only covers regular wallets. A better would add a watch-only only wallet.
ACKs for top commit:
meshcollider:
utACK 4a96e459d733f1b6427221aaa1874ea00f79988a
jb55:
utACK 4a96e459d733f1b6427221aaa1874ea00f79988a
promag:
reACK 4a96e45, rebased and label change since last review.
instagibbs:
code review and light test ACK 4a96e459d7
Tree-SHA512: 4213549888bd309f72bdbba1453218f4a2b07e809100d786a3791897c75468f9092b06fe4b971942b1c228aa75ee7c04971f262ca9a478b42756e056eb534620
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
b007efdf1910db1d38671d6435d2f379bbf847d2 Allow BnB when subtract fee from outputs (Andrew Chow)
db15e71e79b24601853703bebd1c92f4b523fd5f Use BnB when preset inputs are selected (Andrew Chow)
Pull request description:
Currently we explicitly disable BnB when there are preset inputs selected or when the subtract fee from outputs option is enabled. This PR enables BnB for both cases.
Kind of an alternative to #17246 (implements the subtract fee from outputs part of it) and borrows a test from there too.
ACKs for top commit:
instagibbs:
reACK b007efdf19
Sjors:
re-ACK b007efdf1910db1d38671d6435d2f379bbf847d2
Tree-SHA512: 933276b09b2fa2ab43db7f0b98762f06f6f5fa8606195f96aca9fa1cb71ae4ee7156028dd482b1cada82ddd0996a9daf12ea5c152589fdf192cd96cbc51e99df
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
3645e4ca0033bb6365f41ef710111780c139370f Add missing newline in util_ChainMerge test (Russell Yanofsky)
Pull request description:
This was causing a lot of test cases not not be very meaningful because
multiple configuration options were combined into one line.
The changes in test output with this fix make sense and look like:
```diff
- testnet=1 regtest=1 || test
+ testnet=1 regtest=1 || error: Invalid combination of -regtest, -testnet and -chain. Can use at most one.
```
Issue was reported and debugged by
Wladimir J. van der Laan <laanwj@protonmail.com> in
https://github.com/bitcoin/bitcoin/pull/17385#issuecomment-550033222
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
laanwj:
ACK 3645e4ca0033bb6365f41ef710111780c139370f
practicalswift:
ACK 3645e4ca0033bb6365f41ef710111780c139370f -- diff looks correct
Tree-SHA512: ca5bde9b9f553811d4827113f4880d15d7b8f4f1455b95bbf34c9a1512fdd53062f1a2133c50d9b54f94160a1ee77a54bc82681a5f3bf25d2b0d01f8a8e95165
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
fac22fd36b2d9f55dada31cc0da55520431b972a log: Remove GetAdjustedTime from IBD header progress estimation (MarcoFalke)
Pull request description:
This is a "refactor" that shouldn't change behaviour, because the two times are most likely equal. A minimum of 5 outbound peers are needed to adjust the time. And if the time is adjusted, it will be by at most 70 minutes (`DEFAULT_MAX_TIME_ADJUSTMENT`). Thus, the progress estimate should differ by at most 7 blocks.
ACKs for top commit:
laanwj:
Code review ACK fac22fd36b2d9f55dada31cc0da55520431b972a
vincenzopalazzo:
ACK fac22fd36b
Tree-SHA512: bf9f5eef66db0110dd268cf6dbfab64b9c11ba776924f5b386ceae3f2d005272cceb87ebcc96e0c8b854c051514854a2a5af39ae43bad008fac685b5aafaabd0
2e42050b7fc61201f202438e8cd4383a06eb98d5 doc: fix undo data filename (s/undo???.dat/rev???.dat/) (Sebastian Falbesoner)
Pull request description:
This typo was discovered in the course of a review club to #20827, see https://bitcoincore.reviews/20827#l-31.
ACKs for top commit:
shaavan:
ACK 2e42050b7fc61201f202438e8cd4383a06eb98d5
Tree-SHA512: 0c7a00dce24c03bee6d37265d5b4bc97e976c3f3910af1113f967f6298940f892d6fb517f7b154f32ccedb365060314d4d78d5eb2a9c68b25f0859a628209cd3
fa37e798b2660d8e44e31c944a257b55aeef5de2 wallet: Replace confusing getAdjustedTime() with GetTime() (MarcoFalke)
Pull request description:
Setting `nTimeReceived` to the adjusted time has several issues:
* `m_best_block_time` is set to the "unadjusted" time, thus a comparison of the two times is like comparing apples to oranges. In the worst case this opens up an attack vector where remote peers can force a premature re-broadcast of wallet txs.
* The RPC documentation for `"timereceived"` doesn't mention that the network adjusted time is used, possibly confusing users when the time reported by RPC is off by a few seconds compared to their local timestamp.
Fix all issues by replacing the call with `GetTime()`. Also a style fix: Use non-narrowing integer conversion in the RPC method.
ACKs for top commit:
theStack:
Code-review ACK fa37e798b2660d8e44e31c944a257b55aeef5de2
shaavan:
crACK fa37e798b2660d8e44e31c944a257b55aeef5de2
Tree-SHA512: 8d020ba400521246b7aed4b6c41319fc70552e8c69e929a5994500375466a9edac02a0ae64b803dbc6695df22276489561a23bd6e030c44c97d288f7b9b2b3fa
a989f98d240a84b5c798252acaa4a316ac711189 refactor: net: subnet lookup: use single-result LookupHost() (Sebastian Falbesoner)
Pull request description:
plus describe single IP subnet case for more clarity
ACKs for top commit:
jonatack:
utACK a989f98d240a84b5c798252acaa4a316ac711189 the patch rebases cleanly to master, the debug build is green, and it is essentially the same patch as c8991f0251dd2a modulo local variable naming, braced initialization, and a comment
vasild:
ACK a989f98d240a84b5c798252acaa4a316ac711189
Tree-SHA512: 082d3481b1fa5e5f3267b7c4a812954b67b36d1f94c5296fe20110699f053e5042dfa13f728ae20249e9b8d71e930c3b119410125d0faeccdfbdc259223ee3a6
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
5b44a75493a1a098404d5e21dc384e74eae1892e refactor: Remove unused CExt{Pub,}Key (de)serialization methods (Sebastian Falbesoner)
Pull request description:
As pointed out in issue #17130, the serialization/deserialization methods for the classes `CExtKey` and
`CExtPubKey` are only used in the BIP32 unit tests and hence can be removed (see comments https://github.com/bitcoin/bitcoin/issues/17130#issuecomment-543750290, https://github.com/bitcoin/bitcoin/issues/17130#issuecomment-543794408 and https://github.com/bitcoin/bitcoin/issues/17130#issuecomment-543814727).
ACKs for top commit:
practicalswift:
ACK 5b44a75493a1a098404d5e21dc384e74eae1892e -- -60 LOC diff looks correct :)
promag:
ACK 5b44a75493a1a098404d5e21dc384e74eae1892e.
MarcoFalke:
unsigned ACK 5b44a75493a1a098404d5e21dc384e74eae1892e
fjahr:
ACK 5b44a75
jonatack:
Light ACK 5b44a75493a1a098404d5e21dc384e74eae1892e. Built, ran tests and bitcoind. `git blame` shows most of the last changes are from commit 90604f16af in 2015 to add bip32 pubkey serialization.
Tree-SHA512: 6887573b76b9e54e117a076557407b6f7908719b2202fb9eea498522baf9f30198b3f78b87a62efcd17ad1ab0886196f099239992ce7cbbaee79979ffe9e5f2c
73b96c94cb6c2afdee7f151768a96944ecaf9d9b net: Fix uninitialized read in ProcessMessage(...) (practicalswift)
Pull request description:
Fix an uninitialized read in `ProcessMessage(…, "tx", …)` when receiving a transaction we already have.
The uninitialized value is read and used on [L2526 in the case of `AlreadyHave(inv) == true`](d8a66626d6/src/net_processing.cpp (L2494-L2526)).
Proof of concept being run against a `bitcoind` built with MemorySanitizer (`-fsanitize=memory`):
```
$ ./p2p-uninit-read-in-conditional-poc.py
Usage: ./p2p-uninit-read-in-conditional-poc.py <dstaddr> <dstport> <net>
$ bitcoind -regtest &
$ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest
SUMMARY: MemorySanitizer: use-of-uninitialized-value
[1]+ Exit 77 bitcoind -regtest
$
```
Proof of concept being run against a `bitcoind` running under Valgrind (`valgrind --exit-on-first-error`):
```
$ valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest &
$ ./p2p-uninit-read-in-conditional-poc.py 127.0.0.1 18444 regtest
==27351== Conditional jump or move depends on uninitialised value(s)
[1]+ Exit 1 valgrind -q --exit-on-first-error=yes --error-exitcode=1 bitcoind -regtest
$
```
Proof of concept script:
```
#!/usr/bin/env python3
import sys
from test_framework.mininode import NetworkThread
from test_framework.mininode import P2PDataStore
from test_framework.messages import CTransaction, CTxIn, CTxOut, msg_tx
def send_duplicate_tx(dstaddr="127.0.0.1", dstport=18444, net="regtest"):
network_thread = NetworkThread()
network_thread.start()
node = P2PDataStore()
node.peer_connect(dstaddr=dstaddr, dstport=dstport, net=net)()
node.wait_for_verack()
tx = CTransaction()
tx.vin.append(CTxIn())
tx.vout.append(CTxOut())
node.send_message(msg_tx(tx))
node.send_message(msg_tx(tx))
node.peer_disconnect()
network_thread.close()
if __name__ == "__main__":
if len(sys.argv) != 4:
print("Usage: {} <dstaddr> <dstport> <net>".format(sys.argv[0]))
sys.exit(0)
send_duplicate_tx(sys.argv[1], int(sys.argv[2]), sys.argv[3])
```
Note that the transaction in the proof of concept is the simplest possible, but really any transaction can be used. It does not have to be a valid transaction.
This bug was introduced in #15921 ("validation: Tidy up ValidationState interface") which was merged in to `master` 28 days ago.
Luckily this bug was caught before being part of any Bitcoin Core release :)
ACKs for top commit:
jnewbery:
utACK 73b96c94cb6c2afdee7f151768a96944ecaf9d9b
laanwj:
ACK 73b96c94cb6c2afdee7f151768a96944ecaf9d9b, thanks for discovering and reporting this before it ended up in a release.
Tree-SHA512: 7ce6b8f260bcdd9b2ec4ff4b941a891bbef578acf4456df33b7a8d42b248237ec4949e65e2445b24851d1639b10681c701ad500b1c0b776ff050ef8c3812c795
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
cd68594dcdadc195bd2ea9394fa04edfdbdf1149 Only check the hash of transactions loaded from disk (Andrew Chow)
Pull request description:
It feels unnecessary to do a full `CheckTransaction` for every transaction saved in the wallet. It should not be possible for an invalid transaction to get into the wallet in the first place, and if there is any disk corruption, the hash check will catch it.
ACKs for top commit:
MarcoFalke:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149
laanwj:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149
promag:
ACK cd68594dcdadc195bd2ea9394fa04edfdbdf1149, AFAICT the check is not needed, hash comparison gives data integrity.
Tree-SHA512: 5b2e719f76097cfbf125392db6cc6c764355c81f0b7a5b60aee4b06af1afcca80cfd38a3cf5307fd9e2c1afc405f8321929a4552943099a8161e6762965451fb
57e2edea0bfea664e3f12dad2508139eb7f461bc Send amount shows minimum amount placeholder (JeremyCrookshank)
Pull request description:
Noticed that there wasn't a default value for the send amount. However if you put a value in or click the up and down arrows you're unable to get it blank again, so it makes sense that it has a default value. I hope this also makes it more clear that users can send less than 1 BTC if it shows the 8 decimal places
PR:
ACKs for top commit:
promag:
ACK 57e2edea0bfea664e3f12dad2508139eb7f461bc.
GChuf:
ACK 57e2edea0bfea664e3f12dad2508139eb7f461bc
laanwj:
ACK 57e2edea0bfea664e3f12dad2508139eb7f461bc, this is a surprisingly compact solution too
Tree-SHA512: 354590d2a88231b8649f7ae985c8a7864d74ca0e1f8603cb1730ba46747084de90ee6285ce4d39ee04b054fb9cd2d78ebc71146f3af694c37a8a3aff7f051800
7d8d3e6a2ad827fa916e3909a18dedb9f7fdce43 Add tests for util/vector.h's Cat and Vector (Pieter Wuille)
e65e61c812df90a56e3ce4a8e76c4b746766f387 Add some general std::vector utility functions (Pieter Wuille)
Pull request description:
This is another general improvement extracted from #16800 .
Two functions are added are:
* Vector(arg1,arg2,arg3,...) constructs a vector with the specified arguments as elements. The vector's type is derived from the arguments. If some of the arguments are rvalue references, they will be moved into place rather than copied (which can't be achieved using list initialization).
* Cat(vector1,vector2) returns a concatenation of the two vectors, efficiently moving elements when relevant.
Vector generalizes (and replaces) the `Singleton` function in src/descriptor.cpp, and `Cat` replaces the function in bech32.cpp
ACKs for top commit:
laanwj:
ACK 7d8d3e6a2ad827fa916e3909a18dedb9f7fdce43
MarcoFalke:
ACK 7d8d3e6a2ad827fa916e3909a18dedb9f7fdce43 (enjoyed reading the tests, but did not compile)
Tree-SHA512: 92325f14e90d7e7d9d920421979aec22bb0d730e0291362b4326cccc76f9c2d865bec33a797c5c0201773468c3773cb50ce52c8eee4c1ec1a4d10db5cf2b9d2a
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>
eb7b78165966f2c79da71b993c4c4d793e37297f modify p2p_feefilter test to catch rounding error (Gregory Sanders)
6a51f7951716d6d6fc0f9b56028f3a0dd02b61c8 Disallow implicit conversion for CFeeRate constructor (Gregory Sanders)
8e59af55aaf1b196575084bce2448af02d97d745 feefilter: Compute the absolute fee rather than stored rate to match mempool acceptance logic (Gregory Sanders)
Pull request description:
This means we will use the rounding-down behavior in `GetFee` to match both mempool acceptance and wallet logic, with minimal changes.
Fixes https://github.com/bitcoin/bitcoin/issues/16499
Replacement PR for https://github.com/bitcoin/bitcoin/pull/16500
ACKs for top commit:
ajtowns:
ACK eb7b78165966f2c79da71b993c4c4d793e37297f code review only
naumenkogs:
utACK eb7b78165966f2c79da71b993c4c4d793e37297f
achow101:
re ACK eb7b78165966f2c79da71b993c4c4d793e37297f
promag:
ACK eb7b78165966f2c79da71b993c4c4d793e37297f.
Tree-SHA512: 484a11c8f0e825f0c983b1f7e71cf6252b1bba6858194abfe4c088da3bae8a418ec539ef6c4181bf30940e277a95c08d493595d59dfcc6ddf77c65b05563dd7e
5c5d0b62648e1b144b7b93c199f45265dac100e5 Add FoundBlock.found member (Russell Yanofsky)
Pull request description:
This change lets IPC serialization code handle FoundBlock arguments more simply and efficiently. Without this change there was no way to determine from a FoundBlock object whether a block was found or not. So in order to correctly implement behavior of leaving FoundBlock output variables unmodified when a block was not found, IPC code would have to read preexisting output variable values from the local process, send them to the remote process, receive output values back from the remote process, and save them to output variables unconditionally. With FoundBlock.found method, the process is simpler. There's no need to read or send preexisting local output variable values, just to read final output values from the remote process and set them conditionally if the block was found.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). The commit was first part of larger PR #10102.
ACKs for top commit:
fjahr:
Code review ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5
theStack:
Concept and code review ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5
jamesob:
ACK 5c5d0b62648e1b144b7b93c199f45265dac100e5 ([`jamesob/ackr/22215.1.ryanofsky.refactor_add_foundblock`](https://github.com/jamesob/bitcoin/tree/ackr/22215.1.ryanofsky.refactor_add_foundblock))
Zero-1729:
crACK 5c5d0b6
Tree-SHA512: d906e1b7100ff72c3aa06d80bd77673887b2db670ebd52dce7c4f6f557a23a1744c6109308228a37fda6c6ea74f05ba0efecff0ef235ab06ea8acd861fbb8675
faa86b71acefc8f2e366746a1c251888e6e686dd fuzz: Use ConsumeUInt256 helper to simplify rolling_bloom_filter fuzz test (MarcoFalke)
aaaa61fd306e25379e6222e31bf160a6eb04f74e fuzz: Speed up rolling_bloom_filter fuzz test (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Reproducer from OSS-Fuzz (without bug report):
[clusterfuzz-testcase-rolling_bloom_filter-5980807721254912.log](https://github.com/bitcoin/bitcoin/files/6822159/clusterfuzz-testcase-rolling_bloom_filter-5980807721254912.log)
ACKs for top commit:
practicalswift:
cr ACK faa86b71acefc8f2e366746a1c251888e6e686dd
theStack:
Concept and code review ACK faa86b71acefc8f2e366746a1c251888e6e686dd
Tree-SHA512: eace588509dfddb2ba97baf86379fa713fa6eb758184abff676cb95807ff8ff36905eeaddeba05665b8464c35c57e2138f88caec71cbfb255e546bbe76558da0
faafda232e1d4f79ee64dbfee699a8018f25b0bc fuzz: Speed up prevector fuzz target (MarcoFalke)
Pull request description:
Without a size limit on the input data, the runtime is unbounded. Fix this by picking an upper bound on the maximum number of fuzz operations.
Should fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=35981
ACKs for top commit:
practicalswift:
cr ACK faafda232e1d4f79ee64dbfee699a8018f25b0bc
Tree-SHA512: 1bf166c4a99a8ce88bdc030cd6a32ce1da5251b73873772e0e9c001ec2bacafebb183f7c8c88806d0ab633aada2cff8b78791f5c9c0c6f2cc8ef5f0875c4b2ef
aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0 fuzz: Extend addrman fuzz test with deserialize (MarcoFalke)
Pull request description:
Requested on IRC:
```
[18:01] <vasild> => I think there is a good chance fuzzing addrman unserialize will find more bugs
[18:04] <sipa> definitely
ACKs for top commit:
jonatack:
ACK aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0 per `git diff fa74025 aaaa9c6`
vasild:
ACK aaaa9c6019790a1a21a7b4ef01693ac9390ae6d0
Tree-SHA512: f57d0aecf22a933e48d3181d7398218949588dd0de31218d1d28c825649e55fd60b0de6fbc92d2497cf5639a4adc2061c9bf8216546a2be916feac4f03f16e8f
fb7be92b094477131140b58a4e3ae98366b93e76 Mark print-% target as phony. (Dmitry Goncharov)
Pull request description:
.PHONY does not take patterns (such as print-%) as prerequisites.
Have print-% depend on force and mark force as phony.
This change ensures print-% rule works even when there is a file that matches the target.
```
$ # on master
$ make print-host
host=x86_64-pc-linux-gnu
$ touch print-host
$ make print-host
make: 'print-host' is up to date.
$
$ git co mark_print_as_phony
Switched to branch 'mark_print_as_phony'
$ make print-host
host=x86_64-pc-linux-gnu
$ touch force
$ make print-host
host=x86_64-pc-linux-gnu
```
ACKs for top commit:
hebasto:
ACK fb7be92b094477131140b58a4e3ae98366b93e76, tested on Linux Mint 20.2 (x86_64).
Tree-SHA512: b89ae66aa8c7aa6a7ab5f0956f9eb3b3ef9d56994b60dc2a97d498d4c1bba537845c190723e8a10310280b1b35df2cd935cc30aeb76735cac2dc621ad7823772
906d7913117c8f10934b37afa27ae8ac565da042 fuzz: add missing ECCVerifyHandle to base_encode_decode (Andrew Poelstra)
Pull request description:
It is possible to trigger a fuzztest failure in the `base_encode_decode` by asking it to decode any PSBT that has HD keypaths in it. For example, this one
```
cHNidP8BAFUCAAAAASeaIyOl37UfxF8iD6WLD8E+HjNCeSqF1+Ns1jM7XLw5AAAAAAD/////AaBa6gsAAAAAGXapFP/pwAYQl8w7Y28ssEYPpPxCfStFiKwAAAAAAAEBIJVe6gsAAAAAF6kUY0UgD2jRieGtwN8cTRbqjxTA2+uHIgIDsTQcy6doO2r08SOM1ul+cWfVafrEfx5I1HVBhENVvUZGMEMCIAQktY7/qqaU4VWepck7v9SokGQiQFXN8HC2dxRpRC0HAh9cjrD+plFtYLisszrWTt5g6Hhb+zqpS5m9+GFR25qaAQEEIgAgdx/RitRZZm3Unz1WTj28QvTIR3TjYK2haBao7UiNVoEBBUdSIQOxNBzLp2g7avTxI4zW6X5xZ9Vp+sR/HkjUdUGEQ1W9RiED3lXR4drIBeP4pYwfv5uUwC89uq/hJ/78pJlfJvggg71SriIGA7E0HMunaDtq9PEjjNbpfnFn1Wn6xH8eSNR1QYRDVb1GELSmumcAAACAAAAAgAQAAIAiBgPeVdHh2sgF4/iljB+/m5TALz26r+En/vykmV8m+CCDvRC0prpnAAAAgAAAAIAFAACAAAA=
```
which I took straight from the PSBT test vectors. The reason is that in src/psbt.h we call `DeserializeHDKeypaths`, which in turn calls `CPubKey::IsFullyValid`, which in turn asserts that a secp context has been created.
The error appears to be masked on many systems by the definition of `instance_of_eccryptoclosure` in src/script/bitcoinconsensus.cpp, which defines a static object which contains an `ECCVerifyHandle`. If you just comment out that line you can reliably trigger the fuzz test failure, e.g. by creating a file `crash` with the above PSBT, and runnnig
```
ASAN_OPTIONS=symbolize=0:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1 UBSAN_OPTIONS=suppressions=./test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1 FUZZ=base_encode_decode ./src/test/fuzz/fuzz -seed_inputs=crash
```
ACKs for top commit:
practicalswift:
cr ACK 906d7913117c8f10934b37afa27ae8ac565da042
Tree-SHA512: b98b60573c21efe28503fe351883c6f0d9ac99d0dd6f100537b16ac53476617b8a3f899faf0c23d893d34a01b3bbe4a784499ec6f9c7000292e850bed449bd85
9550dffa0c61df6d1591c62d09629b4c5731e1b7 fuzz: Assert roundtrip equality for `CPubKey` (Sebastian Falbesoner)
Pull request description:
This PR is a (quite late) follow-up to #19237 (https://github.com/bitcoin/bitcoin/pull/19237#issuecomment-642203251). Looking at `CPubKey::Serialize` and `CPubKey::Unserialize` I can't think of a scenario where the roundtrip (serialization/deserialization) equality wouldn't hold.
ACKs for top commit:
jamesob:
crACK 9550dffa0c pending CI
Tree-SHA512: 640fb9e777d249769b22ee52c0b15a68ff0645b16c986e1c0bce9742155d14f1be601e591833e1dc8dcffebf271966c6b861b90888a44aae1feae2e0248e2c55
fa483e9f68b8b4171dabb25cc88dc2eada454a99 fuzz: Speed up crypto fuzz target (MarcoFalke)
Pull request description:
May fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34962
Similar solution to https://github.com/bitcoin/bitcoin/pull/22005
ACKs for top commit:
practicalswift:
cr ACK fa483e9f68b8b4171dabb25cc88dc2eada454a99: patch looks correct and rationale makes sense
Tree-SHA512: 3788cf9f6ba0f7a0a217cd3a6a825839689425e99e4d6d657981d291a001b0da7c5abb50a68b4ee1c2a8300b87fb92e4e3ccc1171907792b40251e467c33bd53
d44a261acff40c1c8727d3cc0106bde65a6416d0 Fix issues when `walletdir` is root directory (unknown)
Pull request description:
+ Remove one character less from wallet path
+ After testing lot of random strings with special chars in `wallet_name`, I found that the issue was not related to special characters in the name. Reviewing PR https://github.com/bitcoin/bitcoin/pull/21907 helped me resolve the issue.
**Real issue**: If the path mentioned in `walletdir` is a root directory, first character of the wallet name or path is removed
**Solution**: `if` statement to check `walletdir` is a root directory
Fixes: https://github.com/bitcoin/bitcoin/issues/21510https://github.com/bitcoin/bitcoin/issues/21501
Related PR: https://github.com/bitcoin/bitcoin/pull/20080
Consider the wallet directories `w1` and `w2` saved in `D:\`. Run `bitcoind.exe -walletdir=D:\`, Results for `bitcoin-cli.exe listwalletdir`:
Before this PR:
```
{
"wallets": [
{
"name": "1"
},
{
"name": "2"
}
]
}
```
After this PR:
```
"wallets": [
{
"name": "w1"
},
{
"name": "w2"
}
]
}
```
ACKs for top commit:
ryanofsky:
Code review ACK d44a261acff40c1c8727d3cc0106bde65a6416d0
meshcollider:
utACK d44a261acff40c1c8727d3cc0106bde65a6416d0
Tree-SHA512: b09b00f727407e3771c8694861dae1bfd29d97a0d51ddcb5d9c0111dc618b3fff2f75829cbb4361c54457ee564e94fcefd9e2928262a1c918a2b6bbad724eb55
1be6267ce1ee142c3b90baed1925a82eab6514aa fuzz: don't try and use fopencookie when building for Android (fanquake)
Pull request description:
When building for Android, `_GNU_SOURCE` will be defined:
```bash
/home/ubuntu/android-sdk/ndk/22.1.7171670/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android30-clang++ -dM -E -x c++ - < /dev/null
#define _GNU_SOURCE 1
#define _LP64 1
#define __AARCH64EL__ 1
#define __AARCH64_CMODEL_SMALL__ 1
#define __ANDROID_API__ 30
#define __ANDROID__ 1
#define __ARM_64BIT_STATE 1
.....
```
but it doesn't have the [`fopencookie()` function](https://www.gnu.org/software/libc/manual/html_node/Streams-and-Cookies.html), or define the `cookie_io_functions_t` type, which results in compile failures:
```bash
In file included from test/fuzz/addition_overflow.cpp:7:
./test/fuzz/util.h:388:15: error: unknown type name 'cookie_io_functions_t'
const cookie_io_functions_t io_hooks = {
^
15 warnings and 1 error generated.
```
Just skip trying to use it if we are building for Android. Should fix#22062.
ACKs for top commit:
practicalswift:
cr ACK 1be6267ce1ee142c3b90baed1925a82eab6514aa
Tree-SHA512: d62f63d0624af04b76c7e07b0332c71eca2bf9cd9e096a60aea9e212b7bbc1548e9fa9a76d065ec719bb345fe8726619c3bd2d0631f54d877c82972b7b289321
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
Bad coding in governance code
## What was done?
Use Enums where possible
## How Has This Been Tested?
Make check
## 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)_
## 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)_
## Issue being fixed or feature implemented
Develop doesn't build: ./autogen.sh fails
## What was done?
remove newline
## How Has This Been Tested?
./autogen.sh
## 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)_
## Issue being fixed or feature implemented
Converts some CCriticalSections with Mutexes; other minor refactoring
in
0fce09d1f0
see before
<img width="771" alt="image"
src="https://user-images.githubusercontent.com/6443210/225969163-bb4cee62-3e6a-4224-980a-11b2e0024a60.png">
and after
<img width="766" alt="image"
src="https://user-images.githubusercontent.com/6443210/225969245-e8afcbf6-c112-40c4-9504-82830b005a53.png">
## What was done?
## How Has This Been Tested?
## Breaking Changes
None
## Checklist:
- [ ] 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
**For repository code-owners and collaborators only**
- [x] I have assigned this pull request to a milestone
fa18553d382a7d8c447cd6698b36e293fb7ecf1f fuzz: Remove addrdb fuzz target (MarcoFalke)
Pull request description:
The target has several issues:
* It is named incorrectly (`addrdb`, but it constructs a `CBanEntry`)
* It doesn't do anything meaningful, other than consuming one integer and passing it to a constructor
* It consumes CPU time that can be used for the other targets
* It is redundant with the banman fuzz target
Fix all by removing it.
ACKs for top commit:
amitiuttarwar:
ACK fa18553d382a7d8c447cd6698b36e293fb7ecf1f, thanks for the cleanup
Tree-SHA512: 3f8944d3f80913bf466c03062fed070e96073fb72d0938b2bc9a2586960c86879d6f251e16fd81cfeb4e6685ff9eef6bccb25cd3901b218a100c90f25a3c9240
14e8cf974a7a317796ef8e97e5cf9c355ceff0ee [consensus] MOVEONLY: Move single-sig checking EvalScript code to EvalChecksig (Pieter Wuille)
Pull request description:
This is another small refactor pulled out of the Schnorr/Taproot PR #17977.
This is in preparation for adding different signature verification rules,
specifically tapscript (BIP 342), which interprets opcode 0xac and 0xad
as Schnorr signature verifications.
ACKs for top commit:
sipa:
ACK 14e8cf974a7a317796ef8e97e5cf9c355ceff0ee, verified move-only.
MarcoFalke:
ACK 14e8cf974a7a317796ef8e97e5cf9c355ceff0ee, reviewed with "git show 14e8cf974a7a317796ef8e97e5cf9c355ceff0ee --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space -W" 👆
fjahr:
Code-review ACK 14e8cf974a7a317796ef8e97e5cf9c355ceff0ee, verified that it's move-only.
instagibbs:
code review ACK 14e8cf974a, verified move-only
theStack:
Code-Review ACK 14e8cf974a
jonatack:
ACK 14e8cf974a7a317796ef8e97e5cf9c355ceff0ee
Tree-SHA512: af2efce9ae39d5ec01db5b9ef0ff383fe252ef5f33b3483927308ae17d91a619266cb45951f32ea1ce54807a4c0f052bcdefb47e244465d3a726393221c227b1
13076867981ab36b3549ab4c29583ae8ed12a709 refactor: Use Mutex type for g_cs_recent_confirmed_transactions (Hennadii Stepanov)
Pull request description:
No need the `RecursiveMutex` type for the `g_cs_recent_confirmed_transactions`.
Related to #19303.
ACKs for top commit:
MarcoFalke:
ACK 13076867981ab36b3549ab4c29583ae8ed12a709
vasild:
ACK 13076867
Tree-SHA512: 67f1be10c80ec18d0f80b9f5036e5a20986314da9b9364ef4e193ad1d9f3f4c8e4c2e16253ca79d649ff602d5b8c2aff58d7dd1085841afb760479a4875cffbe
37ae687f95c82f2d64ed880533d158060d4fc3de Add tests for CPubKey serialization/unserialization (Elichai Turkel)
9b8907faded8e4ec312c0dd4b4b15e1793876acd Check size after Unserializing CPubKey (Elichai Turkel)
Pull request description:
Found by practicalswift, closes#19235
Currently all the public API(except the pointer-like API) in CPubKey that sets/constructs a pubkey goes through `CPubKey::Set` which checks if that the length and size match and if not invalidates the key.
This adds the same check to `CPubKey::Unserialize`, sadly I don't see an easy way to just push this to the existing checks in `CPubKey::Set` but it's only a simple condition.
The problem with not invalidating is that if you write a pubkey like: `{0x02,0x00}` it will think the actual length is 33(because of `size()`) and will access uninitialized memory if you call any of the functions on CPubKey.
ACKs for top commit:
practicalswift:
re-ACK 37ae687f95c82f2d64ed880533d158060d4fc3de
jonatack:
Code review re-ACK 37ae687 per `git diff eab8ee3 37ae687` only change since last review at eab8ee3 is passing the `pubkey` param by reference to const instead of by value in `src/test/key_tests.cpp::CmpSerializationPubkey`
MarcoFalke:
ACK 37ae687f95c82f2d64ed880533d158060d4fc3de
Tree-SHA512: 30173755555dfc76d6263fb6a59f41be36049ffae7b4e1b92b922d668f5e5e2331f7374d5fa10d5d59fc53020d2966156905ffcfa8b8129c1f6d0ca062174ff1
