b2ff500fb3e4fa05de366ab1900825bea1f70377 test: add "diamond" unit test to MempoolAncestryTests (Sebastian Falbesoner)
Pull request description:
Approaches #17271 (_Missing Unit Test for Ancestors "diamond"_).
If ancestors are represented more than once (in this case `ta` and `tb`), check that those are not overcounted.
ACKs for top commit:
laanwj:
ACK b2ff500fb3e4fa05de366ab1900825bea1f70377
Tree-SHA512: 82a6573cc7f0e82bf6fcfe207d7ddecbf297d2a203d22e95b73d887e3cb280f45a3c5f649161561c1be1eb560ff81b9b385868f205d1c12284211c2377e5ad99
bd3b0361d Add stop_block out arg to ScanForWalletTransactions (Ben Woosley)
3002d6cf3 Return a status enum from ScanForWalletTransactions (Ben Woosley)
bb24d6865 Make CWallet::ScanForWalletTransactions args and return value const (Ben Woosley)
Pull request description:
Return the failed block as an out arg.
Fixes#11450.
/cc #12275
Tree-SHA512: 6a523e5425ebfe24e664a942ae21c797ccc1281c25b1bf8d02ad95c19dae343fd8051985ef11853474de7628fd6bed5f15190fbc087c3466ce6fdecab37d72a9
543ef7d626 tests: Add missing cs_main locks required when accessing pcoinsdbview, pcoinsTip or pblocktree (practicalswift)
Pull request description:
Add missing `cs_main` locks required when accessing `pcoinsdbview`, `pcoinsTip` or `pblocktree`.
This is a subset of #15192: split up requested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/15192#issuecomment-462827372.
The end goal is to get the corresponding `GUARDED_BY(...)`:s in (see #15192).
Tree-SHA512: 0eb1987dba1a2f1faf0910c421f6d90a20b8a253486eb3301d5bca66d128b19120664e3a8580bdce7b428df817284faf94243250bf561f91d2d31a52d134aa67
223de8d94d6522f795ec3c2e7db27469f24aa68c Document RNG design in random.h (Pieter Wuille)
f2e60ca98530e0a865ff6c6fd3c5633aec11a515 Use secure allocator for RNG state (Pieter Wuille)
cddb31bb0a132afa50b5350196cf26f0064fe3e2 Encapsulate RNGState better (Pieter Wuille)
152146e782d401aa1ce7d989d62306aabc85f22e DRY: Implement GetRand using FastRandomContext::randrange (Pieter Wuille)
a1f252eda87356fa329c838a7bf569808489648f Sprinkle some sweet noexcepts over the RNG code (Pieter Wuille)
4ea8e50837a0932b31a241988fd68d6730a2048a Remove hwrand_initialized. (Pieter Wuille)
9d7032e4f066777c97c58b1394884716e213790a Switch all RNG code to the built-in PRNG. (Pieter Wuille)
16e40a8b562ad849a5f5e8b21ceb375e46038243 Integrate util/system's CInit into RNGState (Pieter Wuille)
2ccc3d3aa346e96206281a391bc29874cf5ee7f4 Abstract out seeding/extracting entropy into RNGState::MixExtract (Pieter Wuille)
aae8b9bf0f4fd2b801ee72cf191588c8b3a67c3c Add thread safety annotations to RNG state (Pieter Wuille)
d3f54d1c82b131d817b20cd9daa75f9d3c9475e1 Rename some hardware RNG related functions (Pieter Wuille)
05fde14e3afe6f7156ebb6df6cd0e3ae12635b89 Automatically initialize RNG on first use. (Pieter Wuille)
2d1cc5093949f8ea9487a68724162c8b39035ad8 Don't log RandAddSeedPerfmon details (Pieter Wuille)
6a57ca91da23c6a5d91399ffc7fc09a99b6d4c76 Use FRC::randbytes instead of reading >32 bytes from RNG (Pieter Wuille)
Pull request description:
This does not remove OpenSSL, but makes our own PRNG the 'main' one; for GetStrongRandBytes, the OpenSSL RNG is still used (indirectly, by feeding its output into our PRNG state).
It includes a few policy changes (regarding what entropy is seeded when).
Before this PR:
* GetRand*:
* OpenSSL
* GetStrongRand*:
* CPU cycle counter
* Perfmon data (on Windows, once 10 min)
* /dev/urandom (or equivalent)
* rdrand (if available)
* From scheduler when idle:
* CPU cycle counter before and after 1ms sleep
* At startup:
* CPU cycle counter before and after 1ms sleep
After this PR:
* GetRand*:
* Stack pointer (which indirectly identifies thread and some call stack information)
* rdrand (if available)
* CPU cycle counter
* GetStrongRand*:
* Stack pointer (which indirectly identifies thread and some call stack information)
* rdrand (if available)
* CPU cycle counter
* /dev/urandom (or equivalent)
* OpenSSL
* CPU cycle counter again
* From scheduler when idle:
* Stack pointer (which indirectly identifies thread and some call stack information)
* rdrand (if available)
* CPU cycle counter before and after 1ms sleep
* Perfmon data (on Windows, once every 10 min)
* At startup:
* Stack pointer (which indirectly identifies thread and some call stack information)
* rdrand (if available)
* CPU cycle counter
* /dev/urandom (or equivalent)
* OpenSSL
* CPU cycle counter again
* Perfmon data (on Windows, once every 10 min)
The interface of random.h is also simplified, and documentation is added.
This implements most of #14623.
Tree-SHA512: 0120e19bd4ce80a509b5c180a4f29497d299ce8242e25755880851344b825bc2d64a222bc245e659562fb5463fb7c70fbfcf003616be4dc59d0ed6534f93dd20
Co-authored-by: Wladimir J. van der Laan <laanwj@gmail.com>
8931a95beca2b959c7ee73b154ce8a69acbe8599 Include util/strencodings.h which is required for IsSpace(...) (practicalswift)
7c9f7907615ff9c10a56ede5a8e47c91cb20fe3b Update KNOWN_VIOLATIONS: Remove fixed violations (practicalswift)
587924f0006d2eb9b8218b6abffe181bb9c27513 Use IsSpace(...) instead of boost::is_space (practicalswift)
c5fd143edb85d0c181e21a429f9e29d12a611831 Use ToLower(...) instead of std::tolower (practicalswift)
e70cc8983c570bbacee37a67df86b1bf959894df Use IsDigit(...) instead of std::isdigit (practicalswift)
Pull request description:
* Use `ToLower(...)` instead of `std::tolower`. `std::tolower` is locale dependent.
* Use `IsDigit(...)` instead of `std::isdigit`. Some implementations (e.g. Microsoft in 1252 codepage) may classify single-byte characters other than `[0-9]` as digits.
* Update `KNOWN_VIOLATIONS`: Remove fixed violations.
* ~~Replace use of locale dependent Boost trim (`boost::trim`) with locale independent `TrimString`.~~
* Use` IsSpace(...)` instead of `boost::is_space`
Tree-SHA512: defed016136b530b723fa185afdbd00410925a748856ba3afa4cee60f61a67617e30f304f2b9991a67b5fe075d9624f051e14342aee176f45fbc024d59e1aa82
4f4993fe2a Remove UBSan suppression (practicalswift)
958e1a307e streams: Remove unused seek(size_t) (practicalswift)
Pull request description:
Fix broken `streams_vector_reader` test. Remove unused `seek(size_t)`.
Before this change the test `streams_vector_reader` triggered an unintended unsigned integer wraparound. It tried so seek using a negative value in `reader.seek(-6)`.
Changes in this PR:
* Fix broken `VectorReader::seek(size_t)` test case
* Remove unused `seek(size_t)`
Tree-SHA512: 6c6affd680626363eef9e496748f2f86a522325abab9d6b13161f41125cdc29ceb36c2c1509c90b8ff108d606df7629e55e094cc2b6253b05a892b81ce176b71
* Merge #13311: Don't edit Chainparams after initialization
6fa901fb47 Don't edit Chainparams after initialization (Jorge Timón)
980b38f8a1 MOVEONLY: Move versionbits info out of versionbits.o (Jorge Timón)
Pull request description:
This encapsulates the "-vbparams" option, which is only meant for regtest, directly on CRegTestParams.
This is a refactor and doesn't change functionality.
Related to https://github.com/bitcoin/bitcoin/pull/8994
Tree-SHA512: 79771d729a63a720e743a9c77d5e2d80369f072d66202a43c1304e83a7d0ef7c6103d4968a03aea9666cc89a7203c618da972124a677b38cfe62ddaeb28f9f5d
* Resolve Merge with #13311
* Incorporated review changes
* Apply suggestions from code review
* Update src/chainparams.cpp
* Update src/chainparams.cpp
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
* Apply suggestions from code review
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
b9f226b41f rpc: Remove cs_main lock from blockToJSON and blockHeaderToJSON (João Barbosa)
343b98cbcd rpc: Specify chain tip instead of chain in GetDifficulty (João Barbosa)
54dc13b6a2 rpc: Fix SoftForkMajorityDesc and SoftForkDesc signatures (João Barbosa)
Pull request description:
Motivated by https://github.com/bitcoin/bitcoin/pull/11913#discussion_r157798157, this pull makes `blockToJSON` and `blockheaderToJSON` free of `cs_main` locks.
Locking `cs_main` was required to access `chainActive` in order to check if the block was in the chain and to retrieve the next block index.
With the this approach, `CBlockIndex::GetAncestor()` is used in a way to check if the block belongs to the specified chain tip and, at the same time, get the next block index.
Tree-SHA512: a6720ace0182c19033bbed1a404f729d793574db8ab16e0966ffe412145611e32c30aaab02975d225df6d439d7b9ef2070e732b16137a902b0293c8cddfeb85f
fa5e373365 validation: Add cs_main locking annotations (MarcoFalke)
fa5c346c5a doc: Add comment to cs_main and mempool::cs (MarcoFalke)
fafe941bdd test: Add missing validation locks (MarcoFalke)
fac4558462 sync: Add RecursiveMutex type alias (MarcoFalke)
Pull request description:
Both the chain state and the transaction pool are validation specific, but access to them is protected by two locks. The two locks have the following semantics:
* Writing to the chain state or adding transactions to the transaction pool -> Take both `cs_main` and `mempool::cs`
* Reading either or removing transactions from the the transaction pool -> Take only the appropriate lock
Tree-SHA512: 6f6e612ffc391904c6434a79a4f3f8de1b928bf0a3e3434b73561037b395e2b40a70a5a4bd8472dd230e9eacc8e5d5374c904a3c509910cf3971dd7ff59a626c
3a0e76fc12b91b2846d756981e15f09b767a9c37 Replace remaining 0 with nullptr in Qt code (Ben Woosley)
9096276e0b2d5b7e19af9a5f3c144ef108ee55e0 Don't use zero as null pointer constant (-Wzero-as-null-pointer-constant) (practicalswift)
Pull request description:
This corrects all violations of `-Wzero-as-null-pointer-constant` identified in the Qt codebase.
These changes are extracted from #15112 as suggested by @MarcoFalke to ease review. This is in service of enabling `-Wzero-as-null-pointer-constant`, which should eliminate this as a concern going forward.
Note there are 2 non-Qt changes: `src/test/allocator_tests.cpp` and `src/wallet/db.cpp`.
Tree-SHA512: 206bd668802147ba42bc413c2d7d259cb59aca9ec1da74a6bf2ca3932e60ae492faacbc61bcee0fd6b4b49a4d59d075b7e5404f0526b36c47718f9b0587e7768
332b3dd7c1 util: Make ToLower and ToUpper take a char (Wladimir J. van der Laan)
edb5bb3500 util: remove unused [U](BEGIN|END) macros (Wladimir J. van der Laan)
7fa238c701 Replace use of BEGIN and END macros on uint256 (Wladimir J. van der Laan)
Pull request description:
Two cleanups in `util/strencodings.h`:
- Remove `[U](BEGIN|END)` macros — The only use of these was in the Merkle tree code with `uint256` which has its own `begin` and `end` methods which are better.
- Make ToLower and ToUpper take a char — Unfortunately, `std::string` elements are (bare) chars. As these are the most likely type to be passed to these functions, make them use char instead of unsigned char. This avoids some casts.
Tree-SHA512: 96c8292e1b588d3d7fde95c2e98ad4e7eb75e7baab40a8e8e8209d4e8e7a1bd3b6846601d20976be34a9daabefc50cbc23f3b04200af17d0dfc857c4ec42aca7
* Merge #13743: refactor: Replace boost::bind with std::bind
cb53b825c2 scripted-diff: Replace boost::bind with std::bind (Chun Kuan Lee)
2196c51821 refactor: Use boost::scoped_connection in signal/slot, also prefer range-based loop instead of std::transform (Chun Kuan Lee)
Pull request description:
Replace boost::bind with std::bind
- In `src/rpc/server.cpp`, replace `std::transform` with simple loop.
- In `src/validation.cpp`, store the `boost::signals2::connection` object and use it to disconnect.
- In `src/validationinterface.cpp`, use 2 map to store the `boost::signals2::scoped_connection` object.
Tree-SHA512: 6653cbe00036fecfc495340618efcba6d7be0227c752b37b81a27184433330f817e8de9257774e9b35828026cb55f11ee7f17d6c388aebe22c4a3df13b5092f0
* Replace boost::bind with std::bind and remove Boost.Bind includes
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
fa61202cae test: Add comment to g_insecure_rand_ctx (MarcoFalke)
fa0d3c4407 test: Undo thread_local g_insecure_rand_ctx (MarcoFalke)
Pull request description:
`thread_local` seems to be highly controversial according to the discussion in #14953, so remove it again from the tests.
Also remove boost::thread_group in the test that uses it, since I am touching it anyway.
Tree-SHA512: 977c1f597e3cfbd0e97d0b037d998fdbc701f62e9a2f57e02dbe1727b63ae8ff478dbd9d3d6dc4ffdfa23f2058b331f04949d51f23a8f55b41ecb75f088f1cbe
fa2510d5c1cdf9c2cd5cc9887302ced4378c7202 Use C++11 default member initializers (MarcoFalke)
Pull request description:
Changes:
* Remove unused constructors that leave some members uninitialized
* Remove manual initialization in each constructor and prefer C++11 default member initializers
This is not a stylistic change, but a change that avoids bugs such as:
* fix uninitialized read when stringifying an addrLocal #14728
* qt: Initialize members in WalletModel #12426
* net: correctly initialize nMinPingUsecTime #6636
* ...
Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b
8794a4b3ae4d34a4cd21a7dee9f694eef7726a4f QA: add test for HKDF HMAC_SHA256 L32 (Jonas Schnelli)
551d489416339dae8f9d896013cd060a21406e2b Add HKDF HMAC_SHA256 L=32 implementations (Jonas Schnelli)
3b64f852e400c552f031697d6a86829dc6e74bd6 QA: add test for CKey::Negate() (Jonas Schnelli)
463921bb649d644f79f9d7f0f96f10aa0d165f76 CKey: add method to negate the key (Jonas Schnelli)
Pull request description:
This adds a limited implementation of `HKDF` (defined by rfc5869) that supports only HMAC-SHA256 and length output of 32 bytes (will be required for v2 transport protocol).
This PR also includes a method to negate a private key which is useful to enforce public keys starting with 0x02 (or 0x03) (a requirement for the v2 transport protocol). The new `CKey::Negate()` method is pretty much a wrapper around `secp256k1_ec_privkey_negate()`.
Including tests.
This is a subset of #14032 and a pre-requirement for the v2 transport protocol.
ACKs for commit 8794a4:
Tree-SHA512: 5341929dfa29f5da766ec3612784baec6a3ad69972f08b5a985a8aafdae4dae36f104a2b888d1f5d1f33561456bd111f960d7e32c2cc4fd18e48358468f26c1a
9c5af58d51 Consolidate redundant implementations of ParseHashStr (Ben Woosley)
Pull request description:
This change:
* adds a length check to all calls to `ParseHashStr`, appropriate given its use to populate
a 256-bit number from a hex str
* allows the caller to handle the failure, which allows for the more
appropriate `JSONRPCError` on failure in `prioritisetransaction` rpc
Relative to #14288
Tree-SHA512: baa791147e5ceb3c30c70df3981aaf807bf7d4a90a0be3625540b59aa4b9a9d303a452bfef18bf167cbb833ef9591b4ef5948bf4a1ce67b421d804ae8d20ea53
faead93c6c test: Make g_insecure_rand_ctx thread_local (MarcoFalke)
Pull request description:
Some tests might spin up several threads and `FastRandomContext` is not thread safe.
Fix that by giving each thread their own randomness context (as opposed to e.g. making `FastRandomContext` thread safe or add locks elsewhere).
Also, add the `g_` prefix to it (according to developer notes), since I am touching it anyway.
Tree-SHA512: c6b61375636dfbb2f8311efe8b47e9fe7c4f8bee9804871243f877545f3117cb6aa8556a2d9b1d1673e46e2e585b695a8ddd235b746b583c3eab962435efe2d1
1ac3c983bf Mark single-argument constructors "explicit" (practicalswift)
Pull request description:
Mark single-argument constructors `explicit`.
Rationale:
* Avoid unexpected implicit promotions.
From the developer notes:
> **By default, declare single-argument constructors explicit.**
> Rationale: This is a precaution to avoid unintended conversions that might arise when single-argument constructors are used as implicit conversion functions.
Tree-SHA512: 7901ed5be808c9d0ecb5ca501e1bc0395987fe1b7941b8548cebac2ff08a14f7dab61fab374a69b9ba29a9295a04245c814325c7f95b97ae558af0780f111dfa
fa4da3c058 [doc] conf: Remove deprecated options from docs, Other cleanup (MarcoFalke)
Pull request description:
Some dumb fixes, like removing the mention that free transactions are still a thing or that rpcuser/pass should be used (as opposed to rpcauth or rpc cookie).
Combined with other fixes because I don't want to create 3 pull requests:
* conf: Remove deprecated options from docs
* Remove only mention of MIT/X11
* Link to developer notes in README.md
Tree-SHA512: 9e45dc6c63037e7618cf3c871d7d9e65b66f1a952f91a6e623d97d90171e29bc40299a06029c4dc21a0f579e68021e3663186bd3a65e3ab333aff711f7dcb2bf
c77f09230b Fix descriptor_tests not checking ToString output of public descriptors (Russell Yanofsky)
Pull request description:
This fixes a minor test bug introduced in #13697 that I noticed while reviewing #14646
Tree-SHA512: efed91200cdff5f86ba5de3461ac00759d285e2905f6cb24cea15d3e23e0581ce5fc14b24a40db093f7ebd662ee1ee2cf67f8798bac1903a78298eda08909cfb
* refactor/bls: misc refactoring and spelling/grammar fixes in bls code
* refactor/evo: misc refactoring and spelling/grammar fixes in evo code
* refactor: some include changes
* refactor: remove redundant `public`
* fix linter
Signed-off-by: pasta <pasta@dashboost.org>
* Sort includes
* Move `class CTxDSIn;`
* Drop unused functions in CBLSWorker
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
* Merge #14771: test: Add BOOST_REQUIRE to getters returning optional
fa21ca09a8 test: Add BOOST_REQUIRE to getters returning optional
(MarcoFalke)
Pull request description:
Usually the returned value is already checked for equality, but for
sanity we might as well require that the getter successfully returned.
* Ports [[nodiscard]] portion of bitcoin#14771
Merges bitcoin/bitcoin#14636: Avoid using numeric_limits for sequence
numbers and lock times.
535203075e Avoid using numeric_limits for sequence numbers and lock
times (Russell Yanofsky)
bafb921507 Remove duplicated code (Hennadii Stepanov)
e4dc39b3bc Replace platform dependent type with proper const
(Hennadii Stepanov)
Pull request description:
Switches to named constants, because numeric_limits calls can be
harder to read and less portable.
Change was suggested by jamesob in
https://github.com/bitcoin/bitcoin/pull/10973#discussion_r213473620
There are no changes in behavior except on some platforms we don't
support (ILP64, IP16L32, I16LP32), where `SignalsOptInRBF` and
`MutateTxAddInput` functions would now work correctly.
fa511e8dad Pass tx pool reference into CheckSequenceLocks (MarcoFalke)
Pull request description:
`CheckSequenceLocks` is called from ATMP and the member function `CTxMemPool::removeForReorg` without passing in the tx pool object that is used in those function's scope and instead using the global `::mempool` instance.
This fix should be refactoring only, since currently there is only one (global) tx pool in normal operation. Though, it fixes hard to track down issues in future settings where more than one mempool exists at a time. (E.g. for tests, rpc or p2p tx relay purposes)
Tree-SHA512: f0804588c7d29bb6ff05ec14f22a16422b89ab31ae714f38cd07f811d7dc7907bfd14e799c4c1c3121144ff22711019bbe9212b39e2fd4531936a4119950fa49
e414486d56b9f06af7aeb07ce13e3c3780c2b69b Do not permit copying FastRandomContexts (Pieter Wuille)
022cf47dd7ef8f46e32a184e84f94d1e9f3a495c Simplify testing RNG code (Pieter Wuille)
fd3e7973ffaaa15ed32e5aeadcb02956849b8fc7 Make unit tests use the insecure_rand_ctx exclusively (Pieter Wuille)
8d98d426116f0178612f14d1874d331042c4c4b7 Bugfix: randbytes should seed when needed (non reachable issue) (Pieter Wuille)
273d02580aa736b7ccea8fce51d90541665fdbd1 Use a FastRandomContext in LimitOrphanTxSize (Pieter Wuille)
3db746beb407f7cdd9cd6a605a195bef1254b4c0 Introduce a Shuffle for FastRandomContext and use it in wallet and coinselection (Pieter Wuille)
8098379be5465f598220e1d6174fc57c56f9da42 Use a local FastRandomContext in a few more places in net (Pieter Wuille)
9695f31d7544778853aa373f0aeed629fa68d85e Make addrman use its local RNG exclusively (Pieter Wuille)
Pull request description:
This improves a few minor issues with the RNG code:
* Avoid calling `GetRand*()` functions (which currently invoke OpenSSL, later may switch to using our own RNG pool) inside loops in addrman, networking code, `KnapsackSolver`, and `LimitOrphanSize`
* Fix a currently unreachable bug in `FastRandomContext::randbytes`.
* Make a number of simplifications to the unit tests' randomness code (some tests unnecessarily used their own RNG or the OpenSSL one, instead of using the unit test specific `insecure_rand_ctx`).
* As a precaution, make it illegal to copy a `FastRandomContext`.
Tree-SHA512: 084c70b533ea68ca7adc0186c39f0b3e0a5c0ae43a12c37286e5d42086e056a8cd026dde61b12c0a296dc80f87fdc87fe303b9e8e6161b460ac2086cf7615f9d
c5b404e8f1973afe071a07c63ba1038eefe13f0f Add functional tests for flexible whitebind/list (nicolas.dorier)
d541fa391844f658bd7035659b5b16695733dd56 Replace the use of fWhitelisted by permission checks (nicolas.dorier)
ecd5cf7ea4c3644a30092100ffc399e30e193275 Do not disconnect peer for asking mempool if it has NO_BAN permission (nicolas.dorier)
e5b26deaaa6842f7dd7c4537ede000f965ea0189 Make whitebind/whitelist permissions more flexible (nicolas.dorier)
Pull request description:
# Motivation
In 0.19, bloom filter will be disabled by default. I tried to make [a PR](https://github.com/bitcoin/bitcoin/pull/16176) to enable bloom filter for whitelisted peers regardless of `-peerbloomfilters`.
Bloom filter have non existent privacy and server can omit filter's matches. However, both problems are completely irrelevant when you connect to your own node. If you connect to your own node, bloom filters are the most bandwidth efficient way to synchronize your light client without the need of some middleware like Electrum.
It is also a superior alternative to BIP157 as it does not require to maintain an additional index and it would work well on pruned nodes.
When I attempted to allow bloom filters for whitelisted peer, my proposal has been NACKed in favor of [a more flexible approach](https://github.com/bitcoin/bitcoin/pull/16176#issuecomment-500762907) which should allow node operator to set fine grained permissions instead of a global `whitelisted` attribute.
Doing so will also make follow up idea very easy to implement in a backward compatible way.
# Implementation details
The PR propose a new format for `--white{list,bind}`. I added a way to specify permissions granted to inbound connection matching `white{list,bind}`.
The following permissions exists:
* ForceRelay
* Relay
* NoBan
* BloomFilter
* Mempool
Example:
* `-whitelist=bloomfilter@127.0.0.1/32`.
* `-whitebind=bloomfilter,relay,noban@127.0.0.1:10020`.
If no permissions are specified, `NoBan | Mempool` is assumed. (making this PR backward compatible)
When we receive an inbound connection, we calculate the effective permissions for this peer by fetching the permissions granted from `whitelist` and add to it the permissions granted from `whitebind`.
To keep backward compatibility, if no permissions are specified in `white{list,bind}` (e.g. `--whitelist=127.0.0.1`) then parameters `-whitelistforcerelay` and `-whiterelay` will add the permissions `ForceRelay` and `Relay` to the inbound node.
`-whitelistforcerelay` and `-whiterelay` are ignored if the permissions flags are explicitly set in `white{bind,list}`.
# Follow up idea
Based on this PR, other changes become quite easy to code in a trivially review-able, backward compatible way:
* Changing `connect` at rpc and config file level to understand the permissions flags.
* Changing the permissions of a peer at RPC level.
ACKs for top commit:
laanwj:
re-ACK c5b404e8f1973afe071a07c63ba1038eefe13f0f
Tree-SHA512: adfefb373d09e68cae401247c8fc64034e305694cdef104bdcdacb9f1704277bd53b18f52a2427a5cffdbc77bda410d221aed252bc2ece698ffbb9cf1b830577
f34c8c466a0e514edac2e8683127b4176ad5d321 Make objects in range declarations immutable by default. Avoid unnecessary copying of objects in range declarations. (practicalswift)
Pull request description:
Make objects in range declarations immutable by default.
Rationale:
* Immutable objects are easier to reason about.
* Prevents accidental or hard-to-notice change of value.
Tree-SHA512: cad69d35f0cf8a938b848e65dd537c621d96fe3369be306b65ef0cd1baf6cc0a9f28bc230e1e383d810c555a6743d08cb6b2b0bd51856d4611f537a12e5abb8b
8ecaee13f70a2062e88a977c950a65d3a2de560f Increase signal to noise in appveyor build output by reducing the MSVC warning count from 12 to 4 (12 is assuming the changes in #14086 are also implemented). (practicalswift)
Pull request description:
Remove unreferenced local variables:
Increase signal to noise in appveyor build output by reducing the MSVC warning count from 12 to 4. 12 is the number of MSVC warnings under our current appveyor setup assuming the changes in #14086 are also implemented.
This makes it easier to spot errors or more important warnings in the verbose appveyor output. MSVC warnings are good, so having access to them in a noise free way (read: without trivial warnings) via appveyor without having to use Windows is really valuable.
See https://github.com/bitcoin/bitcoin/pull/14086#issuecomment-416610313 plus discussion for context.
Before:
```
c:\projects\bitcoin\src\script\script.cpp(272): warning C4018: '>': signed/unsigned mismatch [C:\projects\bitcoin\build_msvc\libbitcoinconsensus\libbitcoinconsensus.vcxproj]
c:\projects\bitcoin\src\rest.cpp(467): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\libbitcoin_server\libbitcoin_server.vcxproj]
c:\projects\bitcoin\src\test\allocator_tests.cpp(147): warning C4312: 'reinterpret_cast': conversion from 'int' to 'void *' of greater size [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\coins_tests.cpp(511): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\coins_tests.cpp(524): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\coins_tests.cpp(722): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\coins_tests.cpp(783): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\crypto_tests.cpp(535): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\dbwrapper_tests.cpp(265): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\net_tests.cpp(118): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\net_tests.cpp(151): warning C4101: 'e': unreferenced local variable [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\scheduler_tests.cpp(57): warning C4305: 'argument': truncation from 'int' to 'bool' [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
```
After:
```
c:\projects\bitcoin\src\script\script.cpp(272): warning C4018: '>': signed/unsigned mismatch [C:\projects\bitcoin\build_msvc\libbitcoinconsensus\libbitcoinconsensus.vcxproj]
c:\projects\bitcoin\src\test\allocator_tests.cpp(147): warning C4312: 'reinterpret_cast': conversion from 'int' to 'void *' of greater size [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\crypto_tests.cpp(535): warning C4334: '<<': result of 32-bit shift implicitly converted to 64 bits (was 64-bit shift intended?) [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
c:\projects\bitcoin\src\test\scheduler_tests.cpp(57): warning C4305: 'argument': truncation from 'int' to 'bool' [C:\projects\bitcoin\build_msvc\test_bitcoin\test_bitcoin.vcxproj]
```
Tree-SHA512: 5051134126c570b8421d57c710f1f1b977600398d2b5e69f8a8bd766b3696f992bf4e3459643b99a6b7e08dee1adc92985ee4d0d52b20755954415cb6f23f2fb
18185b57c32d0a43afeca4c125b9352c692923e9 scripted-diff: batch-recase BanMan variables (Carl Dong)
c2e04d37f3841d109c1fe60693f9622e2836cc29 banman: Add, use CBanEntry ctor that takes ban reason (Carl Dong)
1ffa4ce27d4ea6c1067d8984455df97994c7713e banman: reformulate nBanUtil calculation (Carl Dong)
daae598feb034f2f56e0b00ecfb4854d693d3641 banman: add thread annotations and mark members const where possible (Cory Fields)
84fc3fbd0304a7d6e660bf783c84bed2dd415141 scripted-diff: batch-rename BanMan members (Cory Fields)
af3503d903b1a608cd212e2d74b274103199078c net: move BanMan to its own files (Cory Fields)
d0469b2e9386a7a4b268cb9725347e7517acace6 banman: pass in default ban time as a parameter (Cory Fields)
2e56702ecedd83c4b7cb8de9de5c437c8c08e645 banman: pass the banfile path in (Cory Fields)
4c0d961eb0d7825a1e6f8389d7f5545114ee18c6 banman: create and split out banman (Cory Fields)
83c1ea2e5e66b8a83072e3d5ad6a4ced406eb1ba net: split up addresses/ban dumps in preparation for moving them (Cory Fields)
136bd7926c72659dd277a7b795ea17f72e523338 tests: remove member connman/peerLogic in TestingSetup (Cory Fields)
7cc2b9f6786f9bc33853220551eed33ca6b7b7b2 net: Break disconnecting out of Ban() (Cory Fields)
Pull request description:
**Old English à la Beowulf**
```
Banman wæs bréme --blaéd wíde sprang--
Connmanes eafera Coreum in.
aéglaéca léodum forstandan
Swá bealdode bearn Connmanes
guma gúðum cúð gódum daédum·
dréah æfter dóme· nealles druncne slóg
```
**Modern English Translation**
```
Banman was famed --his renown spread wide--
Conman's hier, in Core-land.
against the evil creature defend the people
Thus he was bold, the son of Connman
man famed in war, for good deeds;
he led his life for glory, never, having drunk, slew
```
--
With @theuni's blessing, here is Banman, rebased. Original PR: https://github.com/bitcoin/bitcoin/pull/11457
--
Followup PRs:
1. Give `CNode` a `Disconnect` method ([source](https://github.com/bitcoin/bitcoin/pull/14605#discussion_r248065847))
2. Add a comment to `std::atomic_bool fDisconnect` in `net.h` that setting this to true will cause the node to be disconnected the next time `DisconnectNodes()` runs ([source](https://github.com/bitcoin/bitcoin/pull/14605#discussion_r248384309))
Tree-SHA512: 9c207edbf577415c22c9811113e393322d936a843d4ff265186728152a67c057779ac4d4f27b895de9729f7a53e870f828b9ebc8bcdab757520c2aebe1e9be35
d6b076c17bc7d513243711563b262524ef0ba74c Drop IsLimited in favor of IsReachable (Ben Woosley)
Pull request description:
These two methods have had the same meaning, but inverted, since
110b62f069. Having one name for a single
concept simplifies the code.
This is a follow-up to #15051.
/cc #7553
Tree-SHA512: 347ceb9e2a55ea06f4c01226411c7bbcade09dd82130e4c59d0824ecefd960875938022edbe5d4bfdf12b0552c9b4cb78b09a688284d707119571daf4eb371b4
003929c0d55532038d5bf6fc0ff4a20628710fae refactor: add [[noreturn]] attribute where applicable (fanquake)
Pull request description:
Similar to #10843. We could build with `-Wmissing-noreturn`, however that would also mean modifying something like `--suppress-external-warnings` to suppress warnings for leveldb, which I don't think we want to do. In any case, the functions where this is applicable are only added/removed very rarely.
ACKs for top commit:
vasild:
ACK 003929c0d55532038d5bf6fc0ff4a20628710fae
Tree-SHA512: 33dfa6547d6b84f38a941f24d4c2effe8fde7b93dbc0b27a9309716420e4a879fdbe689d789fa5439d65f5f78292f89fd9dc1b61c97acf69316dfed954086705
f7264fff0a098f8b6354c7373b8790791c25dd07 Check if Cjdns address is valid (Lucas Ontivero)
Pull request description:
CJDNS addresses start with 0xFC and for that reason if a netaddr was unserialized with network type cjdns but its address prefix is not 0xFC then that netaddr should be considered invalid.
ACKs for top commit:
jonatack:
ACK f7264fff0a098f8b6354c7373b8790791c25dd07
practicalswift:
cr ACK f7264fff0a098f8b6354c7373b8790791c25dd07: patch looks correct
theStack:
ACK f7264fff0a098f8b6354c7373b8790791c25dd07 ✔️
Tree-SHA512: 5300df2ffbbd69c40271b6d8df96cca98eb3e1ee76aba62c9c76025d083788ab1f1332775890c63b06e02ca593863a867cd53956bce5962383e8450487898669
fa2204f6adef493079d1ca5148b0fdc2b55816e6 streams: Accept URef obj for VectorReader unserialize (MarcoFalke)
Pull request description:
Missed in commit 172f5fa738. An URef may collapse into an LRef or RRef depending on context. There is no reason to forbid RRef in `VectorReader::operator>>`, so add it for consistency.
ACKs for top commit:
ryanofsky:
Code review ACK fa2204f6adef493079d1ca5148b0fdc2b55816e6, just expanded test since last review
Tree-SHA512: 09ff4e8a918e15b08cebd8c125d37e78bfb3a635c38546fc8454a97a882b2c81c55ef552243617e78744799d31127e6fbf78c4e319c030480b370aab6f38b645
9a19c9ada53e84d1ee8521b48f656faad48b737a Always define the raii_event_tests test suite (Craig Andrews)
Pull request description:
The test suite must always be defined (even when EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined) so that the test harness doesn't fail due to not being able to find the raii_event_tests test.
This improves upon 95f97f4 actually fixing https://github.com/bitcoin/bitcoin/issues/9493
ACKs for top commit:
MarcoFalke:
ACK 9a19c9ada53e84d1ee8521b48f656faad48b737a 🎹
Tree-SHA512: 3c42f17a9b5d56c8841f3aa9ac19da91c10aff210026266f31f7eb98a62528740d7c518c121452b68e8f801d6c80ecfb627d137ec6ed533289fa3beb08b4f176
b3c4d9bac6910f6c28f6008c5ca7064a315fd2a5 test: rename test suite name "tx_validationcache_tests" to match filename (Sebastian Falbesoner)
Pull request description:
Quoting `src/test/README.md`, '`Adding test cases`':
> "The file naming convention is `<source_filename>_tests.cpp`
> and such files should wrap their tests in a test suite
> called `<source_filename>_tests`."
Currently the unit test source file `txvalidationcache_tests.cpp` contains a unit test suite with the name `tx_validationcache_tests`, which is fixed by this PR. The following shell script shows that this is the only mismatch and for all other unit test source files the test suite names are correct:
```
#!/bin/bash
shopt -s globstar
for test_full_filename in **/*_tests.cpp; do
test_name_file=`basename $test_full_filename .cpp`
test_name_suite=`sed -n "s/^.*TEST_SUITE(\(.*_tests\).*$/\1/p" $test_full_filename`
if [ $test_name_file != $test_name_suite ]; then
echo "TestFilename: $test_name_file != TestSuitname: $test_name_suite"
fi
done
```
ACKs for top commit:
practicalswift:
ACK b3c4d9bac6910f6c28f6008c5ca7064a315fd2a5 -- expected naming is better than unexpected naming :)
kristapsk:
ACK b3c4d9bac6910f6c28f6008c5ca7064a315fd2a5
Tree-SHA512: 29d409b1eb22057ee2cc407508e2580d2bc03f412401df11b8ecf77be5ada6bda8f7d2cb5338c5e079490fa12242c1fd6230a09e47252c1b0d9fe535a828ca4c
a5a2654bbc43b5c208418872e5d4c0acbadda5de test: add missing #include to fix compiler errors (Karl-Johan Alm)
Pull request description:
I believe this fixes AppVeyor errors in master. Will close if that is not the case.
Closes#17976
ACKs for top commit:
fanquake:
ACK a5a2654bbc43b5c208418872e5d4c0acbadda5de - glad the fix turned out to be this simple.
Tree-SHA512: 8fed8c2050d0f435e7ed6db1c2927d5daccc3540c6cf9e57e644d0931a740359550a5270201c893f40200960101f11cd039d807d4ed0190f1e0c674f86fd7290
529d332fbfe633d60845a97e1a06f552bd63d0d4 test: add IsRFC2544 tests (Mark Tyneway)
419ef3b7cc04e3ab26252d7024da847dfd5ab1a3 CNetAddr: fix IsRFC2544 comment (Mark Tyneway)
Pull request description:
The comment describing the functionality of `CNetAddr::IsRFC2544` is incorrect.
46d6930f8c/src/netaddress.h (L57)
It should actually read `198.18.0.0/15` based on [RFC 3330](https://tools.ietf.org/html/rfc3330):
```
198.18.0.0/15 - This block has been allocated for use in benchmark
tests of network interconnect devices. Its use is documented in
[RFC2544].
```
See [RFC 2544](https://tools.ietf.org/html/rfc2544) here.
See the implementation here:
47d981e827/src/netaddress.cpp (L142-L145)
This PR also adds tests for the minimum and maximum values that are valid RFC 2544 addresses.
ACKs for top commit:
practicalswift:
ACK 529d332fbfe633d60845a97e1a06f552bd63d0d4
laanwj:
ACK 529d332fbfe633d60845a97e1a06f552bd63d0d4
promag:
ACK 529d332fbfe633d60845a97e1a06f552bd63d0d4, nit could squash.
jonatack:
ACK 529d332fbfe633d60845a97e1a06f552bd63d0d4
Tree-SHA512: 954a9582856d77564e0ea5fd2e3d287d0cfc4ecfe0588115692d01005e8ca7ad8ab20ff390ded867dc91af2bfb758d4e73a336e6c0b7798846c30a6d69b8ae3d
76303f65f92a0fbe9a90c0e807554a6daa860636 test: add unit test for non-standard txs with wrong nVersion (Dominik Spicher)
Pull request description:
Takes care of one of the missing cases of #17394: nVersion must be within the allowed range.
ACKs for top commit:
instagibbs:
ACK 76303f65f9
Tree-SHA512: 94464f781cf70a5616f7cea2014ae0a97a338c34411cc989c60389de2ce00368374811db78c919bda30e0ebf341fb830998a5e97c124dd8afc8feb726cedfd3a
70ed2ab7ef9e7ebf56f77b7c410a345ff455938f Add unit test for DB creation with unicode path (Aaron Clauson)
Pull request description:
An issue arose when attempting to switch back to the main repo version of leveldb when the bitcoin data directory uses a unicode path. The leveldb windows file IO wrapper was using the *A ANSI win32 calls instead of the Unicode *W ones. This unit test will catch if the path created by leveldb doesn't match what we're expecting. For more info see https://github.com/google/leveldb/issues/755.
ACKs for top commit:
laanwj:
ACK 70ed2ab7ef9e7ebf56f77b7c410a345ff455938f
Tree-SHA512: fc6dbd3aa26a439016e63e8d4d931f218ce99094fc7887a13b54562ad4133047020288ecbcd622a8309f422ee1eda5df50bcb8c8e44442af36ed57b22c069004
fab7d14ea5a4305317d66f35beb3225a07823d42 test: Check that wait_until returns if time point is in the past (MarcoFalke)
Pull request description:
Add an explicit regression test for the condvar bug (#18227), so that this doesn't happen again
ACKs for top commit:
laanwj:
ACK fab7d14ea5a4305317d66f35beb3225a07823d42
Tree-SHA512: 6ec0d0b3945cae87a001e367af34cca1953a8082b4a0d9f8a20d30acd1f36363e98035d4eb173ff786cf6692d352d41f960633415c46394af042eb44e3b5ad71
5710dadf9b282524fddf42c682351cd8022ed7bf test: fix script_p2sh_tests OP_PUSHBACK2/4 missing (kodslav)
Pull request description:
Cleans up #15140 which fixes commit 6b25f29a91 where opcodes were lost in translation.
ACKs for top commit:
laanwj:
code review ACK 5710dadf9b282524fddf42c682351cd8022ed7bf
Tree-SHA512: 3f7fbcaf0dd199626d9ec9fdf3c5b5c5c2a91c4cfe81fae5b1d5662a48e52cf4bd27c94f8f42ebdfe7a076c5d600ada5661a6902b03eb5dc3dc953f4524345ac
4896bacc00549c14f3284f5a2b61fb848ac31be0 Add testcase to simulate bitcoin schema in leveldb (MapleLaker)
Pull request description:
Resurrecting #14125 with updates based on comments of closed PR
ACKs for top commit:
laanwj:
ACK 4896bacc00549c14f3284f5a2b61fb848ac31be0
dongcarl:
ACK 4896bacc00549c14f3284f5a2b61fb848ac31be0
Tree-SHA512: 3290ea7e1e998901d5ee8921d1d76cec399cae30ac1911a45b86826afed47cee1acf92bd6438f1fa11ed785a3b17abdcb1c169bc0419945eda9fe4c089d0b6eb
efd2474d17098c754367b844ec646ebececc7c74 util: CBufferedFile fixes (Larry Ruane)
Pull request description:
The `CBufferedFile` object guarantees its user is able to "rewind" the data stream (that's being read from a file) up to a certain number of bytes, as specified by the user in the constructor. This guarantee is not honored due to a bug in the `SetPos` method.
Such rewinding is done in `LoadExternalBlockFile()` (currently the only user of this object), which deserializes a series of `CBlock` objects. If that function encounters something unexpected in the data stream, which is coming from a `blocks/blk00???.dat` file, it "rewinds" to an earlier position in the stream to try to get in sync again. The `CBufferedFile` object does not actually rewind its file offset; it simply repositions its internal offset, `nReadPos`, to an earlier position within the object's private buffer; this is why there's a limit to how far the user may rewind.
If `LoadExternalBlockFile()` needs to rewind (call `blkdat.SetPos()`), the stream may not be positioned as it should be, causing errors in deserialization. This need to rewind is probably rare, which is likely why this bug hasn't been noticed already. But if this object is used elsewhere in the future, this could be a serious problem, especially as, due to the nature of the bug, the `SetPos()` _sometimes_ works.
This PR adds a unit test for `CBufferedFile` that fails due to this bug. (Until now it has had no unit tests.) The unit test provides good documentation and examples for developers trying to understand `LoadExternalBlockFile()` and for future users of this object.
This PR also adds code to throw an exception from the constructor if the rewind argument is not less than the buffer size (since that doesn't make any sense).
Finally, I discovered that the object is too restrictive in one respect: When the deserialization methods call this object's `read` method, a check ensures that the number of bytes being requested is less than the size of the buffer (adjusting for the rewind size), else it throws an exception. This restriction is unnecessary; the object being deserialized can be larger than the buffer because multiple reads from disk can satisfy the request.
ACKs for top commit:
laanwj:
ACK ~after squash.~ efd2474d17098c754367b844ec646ebececc7c74
mzumsande:
I had intended to follow up earlier on my last comment, ACK efd2474d17098c754367b844ec646ebececc7c74. I reviewed the code, ran tests and did a successful reindex on testnet with this branch.
Tree-SHA512: 695529e0af38bae2af4e0cc2895dda56a71b9059c3de04d32e09c0165a50f6aacee499f2042156ab5eaa6f0349bab6bcca4ef9f6f9ded4e60d4483beab7e4554
