fa749fbea3 rpc: Replace boost::variant with std::variant for RPCArg.m_fallback (MarcoFalke)
Pull request description:
Now that we can use std::variant from the vanilla standard library, drop the third-party boost variant dependency.
Patch is split out from #20480. A step-by-step replacement is possible because we don't have our own `Variant` wrapper and the source code specifies `boost::variant` explicitly.
I think a step-by-step replacement should be preferred, because it simplifies review.
ACKs for top commit:
fjahr:
re-ACK fa749fbea3
Sjors:
re-ACK fa749fbea3
Tree-SHA512: 5e3c12b7d535f73065b4afa8df0a488f78fb25d2234f5ecbf740e624db03a34c35fea100eb7d37e84741721310e6450b7fb4296a2207a7ed1fa24485b3650981
f3ba916e8b lint: ignore gitian keys file for spelling linter (Sebastian Falbesoner)
da289a6c4a lint: update list of spelling linter false positives (Sebastian Falbesoner)
a0022f1cfb test: bump codespell linter version to 2.0.0 (Sebastian Falbesoner)
Pull request description:
This small patch updates the ignore list for the spelling linter script (which uses `codespell`), both removing false-positives that are not relevant anymore and adding new ones. As [suggested by jonatack](https://github.com/bitcoin/bitcoin/pull/20762#issuecomment-750889701)~~, whose last name is now also part of the list :)~~. Also changed the linter script to not check the gitian keys file, as [suggested by hebasto](https://github.com/bitcoin/bitcoin/pull/20817#discussion_r550763409). The codespell version used is bumped to most recent version 2.0.0, which is more aware of some terms that were previously needed in the ignorelist for v1.17.1, see https://github.com/bitcoin/bitcoin/pull/20817#issuecomment-753428669.
Running spelling linter on master branch (repeated findings in the same file are removed to keep the output short):
```
$ ./test/lint/lint-spelling.sh
contrib/gitian-keys/keys.txt:16: Atack ==> Attack
doc/developer-notes.md:1284: inout ==> input, in out
doc/psbt.md:122: Asend ==> Ascend, as end
src/bench/verify_script.cpp:27: Keypair ==> Key pair
src/blockencodings.h:30: Unser ==> Under, unset, unsure, user
src/compressor.h:65: Unser ==> Under, unset, unsure, user
src/core_read.cpp:131: presense ==> presence
src/index/disktxpos.h:21: blockIn ==> blocking
src/net_processing.h:67: anounce ==> announce
src/netaddress.h:486: compatiblity ==> compatibility
src/primitives/transaction.h:35: nIn ==> inn, min, bin, nine
src/qt/bitcoinunits.cpp:101: nIn ==> inn, min, bin, nine
src/rpc/blockchain.cpp:2150: nIn ==> inn, min, bin, nine
src/rpc/misc.cpp:198: nIn ==> inn, min, bin, nine
src/script/bitcoinconsensus.cpp:81: nIn ==> inn, min, bin, nine
src/script/bitcoinconsensus.h:63: nIn ==> inn, min, bin, nine
src/script/interpreter.cpp:1279: nIn ==> inn, min, bin, nine
src/script/interpreter.h:222: nIn ==> inn, min, bin, nine
src/script/sign.cpp:17: nIn ==> inn, min, bin, nine
src/script/sign.h:39: nIn ==> inn, min, bin, nine
src/serialize.h:181: Unser ==> Under, unset, unsure, user
src/signet.cpp:142: nIn ==> inn, min, bin, nine
src/test/base32_tests.cpp:17: fo ==> of, for
src/test/base64_tests.cpp:17: fo ==> of, for
src/test/script_tests.cpp:1509: nIn ==> inn, min, bin, nine
src/test/sighash_tests.cpp:27: nIn ==> inn, min, bin, nine
src/test/validation_tests.cpp:78: excercise ==> exercise
src/undo.h:36: Unser ==> Under, unset, unsure, user
src/validation.cpp:1403: nIn ==> inn, min, bin, nine
src/validation.h:255: nIn ==> inn, min, bin, nine
src/wallet/wallet.cpp:1532: nIn ==> inn, min, bin, nine
src/wallet/walletdb.cpp:429: Crypted ==> Encrypted
test/functional/feature_nulldummy.py:63: unnecssary ==> unnecessary
test/functional/wallet_encryption.py:81: crypted ==> encrypted
test/functional/wallet_upgradewallet.py:36: fpr ==> for, far, fps
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
```
Running spelling linter on PR branch:
```
$ ./test/lint/lint-spelling.sh
src/core_read.cpp:131: presense ==> presence
src/net_processing.h:67: anounce ==> announce
src/netaddress.h:486: compatiblity ==> compatibility
src/test/validation_tests.cpp:78: excercise ==> exercise
src/wallet/walletdb.cpp:429: Crypted ==> Encrypted
test/functional/feature_nulldummy.py:63: unnecssary ==> unnecessary
test/functional/wallet_encryption.py:81: crypted ==> encrypted
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/lint-spelling.ignore-words.txt
```
This list of remaining findings doesn't contain false positives anymore -- the typos are fixed in PR https://github.com/bitcoin/bitcoin/pull/20762.
Happy new year! 🍾
ACKs for top commit:
hebasto:
re-ACK f3ba916e8b, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/20817#pullrequestreview-560632881) review.
jonatack:
ACK f3ba916e8b I don't know if there are any particular issues with bumping codespell to v2.0.0, but locally running the spelling linter and the cirrus job at https://cirrus-ci.com/task/5004066998714368 both LGTM. Thanks for also verifying and removing the unused words from the ignore list.
Tree-SHA512: e92ae6f16c01d4ff3d54f8c3a0ee95e12741f7bfe031d307a785f5cfd8a80525b16b34275f413b914c4a318f5166f9887399c21f2dad9cc7e9be41647042ef37
fad140e311 test: Set correct nValue for multi-op-return policy check (MarcoFalke)
Pull request description:
`CTxOut::nValue` is default-initialized to `-1`. The dust-threshold for `OP_RETURN` outputs is `0`. Thus, the policy failure would be `dust` instead of `multi-op-return`. The test only passes because the dust check is currently not run.
Avoid that confusion by setting the value to `0`, to ensure the dust check passes.
ACKs for top commit:
theStack:
ACK fad140e311
Tree-SHA512: f0c7a68eb2c573d6595b2b129fa8fa2a34fa35c17691f448bf1c54ccf66059c37562e7480cde7b51c4de677038d7717873da4257147a5f60acc8bbcd25fb7e3f
efaf80e9bb fuzz: check that certain script TxoutType are nonstandard (Michael Dietz)
Pull request description:
- Every transaction of type NONSTANDARD must not be a standard script
- The only know types of nonstandard scripts are NONSTANDARD and certain NULL_DATA and MULTISIG scripts
When reviewing https://github.com/bitcoin/bitcoin/pull/20761 I figured this is very similar and might also be good to have
ACKs for top commit:
MarcoFalke:
ACK efaf80e9bb
Tree-SHA512: 6f563ee3104ea9d2633aad95f1d003474bea759d0f22636c37aa91b5536a6ff0800c42447285ca8ed12f1b3699bf781dae1e5e0a3362da578749cd3164a06ea4
0dade9154d fuzz: remove no-longer-necessary packages from fuzzbuzz config (fanquake)
Pull request description:
I take it this is actively being used, given [comments in #20560](https://github.com/bitcoin/bitcoin/pull/20560#issuecomment-743747317); so remove old dependencies from setup.
ACKs for top commit:
practicalswift:
ACK 0dade9154d
Tree-SHA512: 781466776575e6051d0dddf4101bd057e484648f63e8e967240fefbf4b5832cacda6f6543708a0c368214a1efe0d60d371da78d7a920646cb93f1a4752aaf639
faaa4f2b6a refactor: Remove nMyStartingHeight from CNode/Connman (MarcoFalke)
Pull request description:
CNode and CConnman keep track of the active chain height when CNodes have been created, but apart from serializing the int once (when sending a version message), it is unused. So it can simply be removed in favor of a single int in PeerMan that can do the same.
ACKs for top commit:
jnewbery:
utACK faaa4f2b6a
theStack:
re-ACK faaa4f2b6a⚓
Tree-SHA512: 713919e49e0f799c4a5a2e5ab07d1ca1c92d4049829680c4f436f9b3b545c548ddbee78ff80611de50cc91a116a416bcc4602e80ee64953597dc04ca15f9806d
86c495223f net: add CNode::IsInboundOnion() public getter and unit tests (Jon Atack)
6609eb8cb5 net: assert CNode::m_inbound_onion is inbound in ctor (Jon Atack)
993d1ecd19 test, fuzz: fix constructing CNode with invalid inbound_onion (Jon Atack)
Pull request description:
The goal of this PR is to be able to depend on `m_inbound_onion` in AttemptToEvictConnection in #20197:
- asserts `CNode::m_inbound_onion` is inbound in the CNode ctor to have a validity check at the class boundary
- fixes a unit test and a fuzz utility that were passing invalid inbound onion values to the CNode ctor
- drops an unneeded check in `CNode::ConnectedThroughNetwork()` for its inbound status
- adds a public getter `IsInboundOnion()` that also allows unit testing it
- adds unit test coverage
ACKs for top commit:
sipa:
utACK 86c495223f
LarryRuane:
ACK 86c495223f
vasild:
ACK 86c495223f
MarcoFalke:
review ACK 86c495223f🐍
Tree-SHA512: 21109105bc4e5e03076fadd489204be00eac710c9de0127708ca2d0a10a048ff81f640f589a7429967ac3eb51d35fe24bb2b12e53e7aa3efbc47aaff6396d204
ccc8d5513f doc: Update license year range to 2021 (Emil Engler)
Pull request description:
See #17801, #15061
The same procedure as every year. Happy new year to all of you :)
Top commit has no ACKs.
Tree-SHA512: 59ca9ba43d3722d5e2ada04fe6c361058a72c84b8437f6331f45e361d7ebf0f3586ccb02dc72af12b9d94f766897f0f041ce3a90efe95eeff22217f9f964ffc6
fa0074e2d8 scripted-diff: Bump copyright headers (MarcoFalke)
Pull request description:
Needs to be done because no one has removed the years yet
ACKs for top commit:
practicalswift:
ACK fa0074e2d8
Tree-SHA512: 210e92acd7d400b556cf8259c3ec9967797420cfd19f0c2a4fa54cb2b3d32ad9ae27e771269201e7d554c0f4cd73a8b1c1a42c9f65d8685ca4d52e5134b071a3
4ddbcd0d9a fuzz: Add coverage for CDataStream consumer (practicalswift)
546a0764f3 fuzz: Fill various small fuzzing gaps (practicalswift)
Pull request description:
Fill various small fuzzing gaps.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
review ACK 4ddbcd0d9a
Tree-SHA512: d20f2cc0172f39948673846d088121782f39b4556df8b38fa14859cfa062c1519d18ee9601d4503ef1ba9613976cc5349c1fc0f0b9601a3d68127ffce1b1854e
454a4088a8 [doc] Add release notes for removed getpeerinfo fields. (Amiti Uttarwar)
b1a936d4ae [rpc] Remove deprecated "whitelisted" field from getpeerinfo (Amiti Uttarwar)
094c3beaa4 [rpc] Remove deprecated "banscore" field from getpeerinfo (Amiti Uttarwar)
537053336f [rpc] Remove deprecated "addnode" field from getpeerinfo (Amiti Uttarwar)
Pull request description:
This PR removes support for 3 fields on the `getpeerinfo` RPC that were deprecated in v0.21- `addnode`, `banscore` & `whitelisted`.
ACKs for top commit:
sipa:
utACK 454a4088a8
jnewbery:
ACK 454a4088a8.
Tree-SHA512: ccc0e90c0763eeb8529cf0c46162dbaca3f7773981b3b52d9925166ea7421aed086795d56b320e16c9340f68862388785f52a9b78314865070917b33180d7cd6
1ef2138c0d lint: run mypy over contrib/devtools (fanquake)
Pull request description:
wumpus mentioned on IRC that we don't currently run `mypy` over the `contrib/devtools` directory, and that it would likely be worthwhile given #20434. This just adds that dir to the linter, as well as some missing annotations to fix existing errors. Note that now we require Python 3.6 we can make use of variable annotations.
master (patched to check contrib devtools):
```bash
test/lint/lint-python.sh
contrib/devtools/symbol-check.py:154: error: Incompatible types in assignment (expression has type "List[str]", variable has type "str")
contrib/devtools/circular-dependencies.py:35: error: Need type annotation for 'deps' (hint: "deps: Dict[<type>, <type>] = ...")
contrib/devtools/circular-dependencies.py:67: error: Need type annotation for 'closure' (hint: "closure: Dict[<type>, <type>] = ...")
Found 4 errors in 3 files (checked 187 source files)
```
I haven't quite gone as far as to add annotations like
```python
CHECKS: Dict[str, List[Tuple[str, Callable[[Any], bool]]]] = {...
```
to `symbol-check.py`.
ACKs for top commit:
laanwj:
ACK 1ef2138c0d
Tree-SHA512: a58c2ece588c640289dc1d35dad5b1b8732788272daa0965d6bf44ee8a7f7c8e8585f94d233ac41c84b9ffcfc97841a00fe2c9acba41f58fd164f01de4b6512b
5e0dedb111 build: Define .INTERMEDIATE target once only (Hennadii Stepanov)
Pull request description:
A new warning was introduced in 22437fc72e (#20470):
```
$ ./autogen.sh
...
Makefile.am:335: warning: .INTERMEDIATE was already defined in condition !BUILD_DARWIN, which is included in condition TRUE ...
Makefile.am:139: ... '.INTERMEDIATE' previously defined here
...
```
Fixed in this PR.
ACKs for top commit:
jonatack:
Tested and very light review ACK 5e0dedb111
Tree-SHA512: ecf8de79ba394c36ee84e0b8d3ba78587e0f856259e9731e6bbb38d0baebfd083eb44d7ef6a386dd9e4508dd64fec1c2b9a007e175fbd4d986e845b1c300a649
faccf8b1e1 refactor: Enable -Wswitch for FeeEstimateHorizon (MarcoFalke)
Pull request description:
This enables the `-Wswitch` compiler warning for `FeeEstimateHorizon` by removing the `default` case in `switch` statements.
ACKs for top commit:
practicalswift:
cr ACK faccf8b1e1
jonatack:
ACK faccf8b1e1
hebasto:
ACK faccf8b1e1, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 63a8dff6e8dead149ec2fa8319e7ff41022c9534d423d3086fd8f22be073dc4915f74c7fe9139ee681a8204730cf58c80ef40c93fb33032d586e68b4f78f557d
40fdb2a212 test: Fix Comment Typo in BitcoinTestFramework (Joel Klabo)
Pull request description:
Missing "override" in comment describing use of set_test_params
ACKs for top commit:
michaelfolkson:
ACK 40fdb2a212
Tree-SHA512: bf893a0d5f8dc86a3ec2eaf48cd7c0f0f832f3b3d254b3d99953336db7e294571b1d2c8686030bf8a27cbe67b1a85a54e53ebefb2e57d6d8d6ac864a15dce4e7
667d203687 [doc] Add permissions to the getpeerinfo help. (Amiti Uttarwar)
Pull request description:
This field was previously being returned, but missing from the RPCHelpMan. This PR uses the existing `NET_PERMISSIONS_DOC` to inform RPC users about this field.
```
"permissions" : [ (json array) Any special permissions that have been granted to this peer
"str", (string) bloomfilter (allow requesting BIP37 filtered blocks and transactions),
noban (do not ban for misbehavior; implies download),
forcerelay (relay transactions that are already in the mempool; implies relay),
relay (relay even in -blocksonly mode, and unlimited transaction announcements),
mempool (allow requesting BIP35 mempool contents),
download (allow getheaders during IBD, no disconnect after maxuploadtarget limit),
addr (responses to GETADDR avoid hitting the cache and contain random records with the most up-to-date info).
...
],
```
ACKs for top commit:
Sjors:
tACK 667d203687
Tree-SHA512: 973631b41d35d6333e3cb06b35277de869110f6ad6498c7e74f00c75202e8de1788a48755c21ac964903e5e6050a5e769a63866211aec9004cd665a727a54a3c
5a9ee0869b tests: Add fuzzing harness for node eviction logic (practicalswift)
Pull request description:
Add fuzzing harness for node eviction logic.
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
cr ACK 5a9ee0869b
Tree-SHA512: c2401d22134867e23dab1ba94ae7ef36fdf52aa0588fdc4705d9cb765ddf979fd775fdf153ce2359f1bc1787cf60bf0ebcd47c7aa29c672e6a253fa58cac292d
c119ba3c9b [doc] clarify getdata limit after #14897 (Michael Polzer)
Pull request description:
GETDATA is limited to blocks and transactions now and can't be used for other non-block data
ACKs for top commit:
laanwj:
ACK c119ba3c9b
theStack:
ACK c119ba3c9b
benthecarman:
ACK c119ba3c9b
Tree-SHA512: d6e9c109bcce4ef004ec83a9ec591163279476524dec97ed5f5c34e322dca35af66a168f0878ff972bbcec79d81623903f3619fedf8f88cdced3f3f66a779173
Pieter Wuille
