* 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
41d484d5c88f057e75cfdd8b88587b9a12a61744 doc: Delete stale URL in test README (Michael Folkson)
Pull request description:
The resource on the Boost unit test framework previously linked to in src/test/README.md was a stale URL.
Instead of deleting it, I've replaced it with an alternative resource on the framework on [boost.org](https://www.boost.org/doc/libs/1_45_0/libs/test/doc/html/utf/tutorials.html).
ACKs for top commit:
promag:
ACK 41d484d5c88f057e75cfdd8b88587b9a12a61744.
hebasto:
ACK 41d484d5c88f057e75cfdd8b88587b9a12a61744, the removed link is really obsolete.
fanquake:
ACK 41d484d5c88f057e75cfdd8b88587b9a12a61744 - Thanks.
Tree-SHA512: 764f12548441bde615f77b7a2ca7c5188b4ab936972d16b84960fbd8604d4cbd224415bc59ce839e7e63293aa84fd97f31a69e38734e531231cdb0e148d2e1bd
5c3c24cf9eab7bf19a9201599e93955cace5c154 test: remove redundant setup in addrman_tests (zenosage)
Pull request description:
#10765 make this default behavior. No reason to keep these line.
Top commit has no ACKs.
Tree-SHA512: 545eea9c2d0741a75708f288f2c8752534ecaa6d54a9d014ef9afa295b0d075007704b64809eec090023703f47753e8ec755d22c9ccecf57b75f6898f6b708dd
f8995807e4 tests: Make coins_tests/updatecoins_simulation_test deterministic (practicalswift)
Pull request description:
Make `coins_tests/updatecoins_simulation_test` deterministic.
Before:
```
$ contrib/devtools/test_deterministic_coverage.sh 1000
[2019-06-15 05:36:20] Measuring coverage, run #1 of 1000
[2019-06-15 05:38:05] Measuring coverage, run #2 of 1000
[2019-06-15 05:39:49] Measuring coverage, run #3 of 1000
[2019-06-15 05:41:38] Measuring coverage, run #4 of 1000
[2019-06-15 05:43:16] Measuring coverage, run #5 of 1000
...
[2019-06-16 18:25:23] Measuring coverage, run #880 of 1000
[2019-06-16 18:27:12] Measuring coverage, run #881 of 1000
[2019-06-16 18:29:33] Measuring coverage, run #882 of 1000
[2019-06-16 18:33:00] Measuring coverage, run #883 of 1000
[2019-06-16 18:35:32] Measuring coverage, run #884 of 1000
The line coverage is non-deterministic between runs. Exiting.
The test suite must be deterministic in the sense that the set of lines executed at least
once must be identical between runs. This is a necessary condition for meaningful
coverage measuring.
--- gcovr.run-1.txt 2019-06-15 05:38:05.282359029 +0200
+++ gcovr.run-884.txt 2019-06-16 18:37:23.518298374 +0200
@@ -269,7 +269,7 @@
test/bloom_tests.cpp 320 320 100%
test/bswap_tests.cpp 13 13 100%
test/checkqueue_tests.cpp 223 222 99% 169
-test/coins_tests.cpp 478 472 98% 52,68,344-345,511,524
+test/coins_tests.cpp 478 474 99% 52,68,511,524
test/compilerbug_tests.cpp 18 18 100%
test/compress_tests.cpp 27 27 100%
test/crypto_tests.cpp 268 268 100%
@@ -401,5 +401,5 @@
zmq/zmqpublishnotifier.h 5 0 0% 12,31,37,43,49
zmq/zmqrpc.cpp 23 3 13% 16,18,20,23,33-35,37,40-47,51,62,64-65
------------------------------------------------------------------------------
-TOTAL 53323 28305 53%
+TOTAL 53323 28307 53%
------------------------------------------------------------------------------
```
After:
```
$ contrib/devtools/test_deterministic_coverage.sh 1000
[2019-06-15 05:36:20] Measuring coverage, run #1 of 1000
[2019-06-15 05:38:05] Measuring coverage, run #2 of 1000
[2019-06-15 05:39:49] Measuring coverage, run #3 of 1000
[2019-06-15 05:41:38] Measuring coverage, run #4 of 1000
[2019-06-15 05:43:16] Measuring coverage, run #5 of 1000
...
$
```
ACKs for commit f89958:
MarcoFalke:
ACK f8995807e4f08fd0266899e3e227903f06da6ab1 (checked that the randomness state of g_insecure_rand_ctx is the same after three test runs)
Tree-SHA512: 796d362b050c5750e351de1126b62f0f2c8e2d712cf01b6e1a3e2cc6ef92fa68439a32fc24c76d34bce4d553aee4ae4ea88a036c56eb9e25979649a19c59c3e5
2c448d6bc7 parameterize hard coded numbers referring to miner conf window (Jordan Baczuk)
Pull request description:
Replace hard coded values (eg. 2016) with `mainnetParams.nMinerConfirmationWindow` where appropriate. This parameterizes hard coded values in the unit test that refer to the `Miner Confirmation Window`, which currently is `2016`. This includes values not exactly 2016 but which were derived from it. Also changed `int` to `uint32_t` where appropriate to avoid compiler warnings. This makes one source of truth, and also helps people who might be adjusting this value in testing so the unit tests don't break.
ACKs for commit 2c448d:
Tree-SHA512: 9262e0b89c1baf7857b49fe2221b2b00f948f61317b321c4871a9182a86d6f8aadeb59d6b133e8a213cc9b31b4a417888fb1ad31caef16ccbbab1de33c4b8459
3ccfa34b32 convert C-style (void) parameter lists to C++ style () (Arvid Norberg)
Pull request description:
In C, an empty parameter list, `()`, means the function takes any arguments, and `(void)` means the function does not take any parameters.
In C++, an empty parameter list means the function does not take any parameters.
So, C++ still supports `(void)` parameter lists with the same semantics, why change to `()`?
1. removing the redundant `void` improves signal-to-noise ratio of the code
2. using `(void)` exposes a rare inconsistency in that a template taking a template `(T)` parameter list, cannot be instantiated with `T=void`
Tree-SHA512: be2897b6c5e474873aa878ed6bac098382cd21866aec33752fe40b089a6331aa6263cae749aba1b4a41e8467f1a47086d32eb74abaf09927fd5a2f44a4b2109a
# Conflicts:
# src/qt/rpcconsole.cpp
f6ee177f7 Remove unused AES-128 code (practicalswift)
Pull request description:
Remove unused AES-128 code.
As far as I can tell this AES-128 code has never been in use in the project (outside of testing/benchmarking).
The AES-256 code is used in `CCrypter::Encrypt`/`CCrypter::Decrypt` (`src/wallet/crypter.cpp`).
Trivia: 0.15% of the project's C++ LOC count (excluding dependencies) is trimmed off:
```
$ LOC_BEFORE=$(git grep -I "" HEAD~1 -- "*.cpp" "*.h" ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/" | wc -l)
$ LOC_AFTER=$(git grep -I "" -- "*.cpp" "*.h" ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/" | wc -l)
$ bc <<< "scale=4; ${LOC_AFTER}/${LOC_BEFORE}"
.9985
```
:-)
Tree-SHA512: 9588a3cd795a89ef658b8ee7323865f57723cb4ed9560c21de793f82d35e2835059e7d6d0705e99e3d16bf6b2a444b4bf19568d50174ff3776caf8a3168f5c85
fa694f706c37a1c512a7c346591a63c5e09ee239 test: Add tests for truncated scripts (MarcoFalke)
Pull request description:
Previously not covered by any test
Tree-SHA512: 9f99659bdf3947271074938456a2fe64f5b39fc868e9aa474cec199a536ae5d7428f1cfa7f361936b71b09ee4c426261e6b25668fa77b8416b30dbe4ddb357f0
cf4b0327ed92ca8a1533cdf6c2b0015fd9b56397 Use std::numeric_limits<UNSIGNED>::max()) instead of (UNSIGNED)-1 (practicalswift)
6b82fc59eb19004e54f910261a40d5e1b9e44b42 Use const in COutPoint class (Hennadii Stepanov)
Pull request description:
Refactoring:
- all cases of using `(uint32_t) -1` in `COutPoint` class are replaced with const;
- also all remaining instances of `(UNSIGNED)-1` transformed to `std::numeric_limits<UNSIGNED>::max()` (by @practicalswift).
Tree-SHA512: fc7fe9838b6e5136d8b97ea3d6f64c4aaa1215f4369832df432cab017396620bb6e30520a64180ceab6de222562ac11eab243a78dfa5a658ba018835a34caa19
9e0a514112 Add compile time checking for all cs_main runtime locking assertions (practicalswift)
Pull request description:
Add compile time checking for `cs_main` runtime locking assertions.
This PR is a subset of #12665. The PR was broken up to make reviewing easier.
The intention is that literally all `EXCLUSIVE_LOCKS_REQUIRED`/`LOCKS_EXCLUDED`:s added in this PR should follow either directly or indirectly from `AssertLockHeld(…)`/`AssertLockNotHeld(…)`:s already existing in the repo.
Consider the case where function `A(…)` contains `AssertLockHeld(cs_foo)` (without
first locking `cs_foo` in `A`), and that `B(…)` calls `A(…)` (without first locking `cs_main`):
* It _directly_ follows that: `A(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation.
* It _indirectly_ follows that: `B(…)` should have an `EXCLUSIVE_LOCKS_REQUIRED(cs_foo)` annotation.
Tree-SHA512: 120e7410c4c223dbc7d42030b1a19e328d01a55f041bb6fb5eaac10ac35cb0c5d469b9b3bda6444731164c73b88ac6495a00890672b107d9305e891571f64dd6
# Conflicts:
# src/validation.cpp
# src/validation.h
# src/wallet/feebumper.cpp
# src/wallet/rpcwallet.cpp
# src/wallet/wallet.h
# Conflicts:
# src/wallet/wallet.h
4b7091a842 Replace median fee rate with feerate percentiles (Marcin Jachymiak)
Pull request description:
Currently, the `medianfeerate` statistic is calculated from the feerate of the middle transaction of a list of transactions sorted by feerate.
This PR instead uses the value of the 50th percentile weight unit in the block, and also calculates the feerate at the 10th, 25th, 75th, and 90th percentiles. This more accurately corresponds with what is generally meant by median feerate.
Tree-SHA512: 59255e243df90d7afbe69839408c58c9723884b8ab82c66dc24a769e89c6d539db1905374a3f025ff28272fb25a0b90e92d8101103e39a6d9c0d60423a596714
e306be742932d4ea5aca0ea4768e54b2fc3dc6a0 Use 72 byte dummy signatures when watching only inputs may be used (Andrew Chow)
48b1473c898129a99212e2db36c61cf93625ea17 Use 71 byte signature for DUMMY_SIGNATURE_CREATOR (Andrew Chow)
18dfea0dd082af18dfb02981b7ee1cd44d514388 Always create 70 byte signatures with low R values (Andrew Chow)
Pull request description:
When creating signatures for transactions, always make one which has a 32 byte or smaller R and 32 byte or smaller S value. This results in signatures that are always less than 71 bytes (32 byte R + 32 byte S + 6 bytes DER + 1 byte sighash) with low R values. In most cases, the signature will be 71 bytes.
Because R is not mutable in the same way that S is, a low R value can only be found by trying different nonces. RFC 6979 for deterministic nonce generation has the option to specify additional entropy, so we simply use that and add a uin32_t counter which we increment in order to try different nonces. Nonces are sill deterministically generated as the nonce used will the be the first one where the counter results in a nonce that results in a low R value. Because different nonces need to be tried, time to produce a signature does increase. On average, it takes twice as long to make a signature as two signatures need to be created, on average, to find one with a low R.
Having a fixed size signature makes size calculations easier and also saves half a byte of transaction size, on average.
DUMMY_SIGNATURE_CREATOR has been modified to produce 71 byte dummy signatures instead of 72 byte signatures.
Tree-SHA512: 3cd791505126ce92da7c631856a97ba0b59e87d9c132feff6e0eef1dc47768e81fbb38bfbe970371bedf9714b7f61a13a5fe9f30f962c81734092a4d19a4ef33
d5f745a5c7 trivial: correct typos (Varunram)
3be70ba400 trivial: Fixed typos and cleaned up language (William Robinson)
Pull request description:
This rebases and fixes some of the outstanding nits in #13010. Let either merge quickly or close for now.
Tree-SHA512: 4cc1a5f854f2d6a19332334e2608a19e2be6b97dc09114c8186237ea77ee4b62372ebf6841a61cca548cedb47f0e6f11d4c0aba51a71949cd5aff8cef88204d6
417b6c1d2990ffc78c029442e027797d724a101f bitcoinconsensus: invalid flags should be set to bitcoinconsensus_error type, add test cases covering bitcoinconsensus error codes (Thomas Kerin)
Pull request description:
A check was added to the bitcoinconsensus verify_script codepath to ensure that callers only used _exposed_ interpreter flags. I think this error should be written to `bitcoinconsensus_err* err` and not returned by verify_script?
I modified the check so it indicates the error using *err like the others, and added tests covering the error codes.
Tree-SHA512: 8ab370e56956a7d4740f83475e6078774affd663ac92383a02b85295da550f1b4f7a7a68f32ed5c5bcb39d98e2f15ec0b76de8399887e7763eb7c1e21d131093
fa84723e73 amount: Move CAmount CENT to unit test header (MarcoFalke)
Pull request description:
`CAmount` is currently not type-safe. Exporting a constant (`CENT`) that is commonly not referred to by that name might be confusing. `CENT` is only used in two places prior to this commit (`ParseMoney` and `MIN_CHANGE`). So replace these with constants relative to `COIN` and move `CENT` to the unit test header.
Tree-SHA512: 5273e96d8664ced6ae211abde2e20bc763e6e99f89404eec02c621f29e1d235e5f9b1ade933743843fae16fc24b643f883deda9221e3d9fd31229d2ab63a914f
ed12d5df1ba52b5ef3dd3799de26bb5e1d3fc654 index: Fix for indexers skipping genesis block. (Jim Posen)
Pull request description:
This fixes a bug where indexers would skip processing of the genesis block. Preserves the current behavior of omitting genesis block transaction from the index.
Tree-SHA512: 092fd3d629bf1ef279566217c668cc913a8b8e012d811d0e544231894c49a0c0c179537ac4727c39b9bf407479541745d79c4e118db6f0795a2b848d0fe62cbf
ec3073a274bf7affe1b8c87a10f75d126f5ac027 index: Move index DBs into index/ directory. (Jim Posen)
89eddcd365e9a2218648f5cc5b9f22b28023f50a index: Remove TxIndexDB from public interface of TxIndex. (Jim Posen)
2318affd27de436ddf9d866a4b82eed8ea2e738b MOVEONLY: Move BaseIndex to its own file. (Jim Posen)
f376a4924109af2496b5fd16a787299eb039f1c8 index: Generalize logged statements in BaseIndex. (Jim Posen)
61a1226d87d80234b2be123c5cad07534c318cfb index: Extract logic from TxIndex into reusable base class. (Jim Posen)
e5af5fc6fb4658599b940d1d50853129b31b8766 db: Make reusable base class for index databases. (Jim Posen)
9b0ec1a7f9ffae816fd5ca32ff7e7559640b6f6d db: Remove obsolete methods from CBlockTreeDB. (Jim Posen)
Pull request description:
This refactors most of the logic in TxIndex into a reusable base class for other indices. There are two commits moving code between files, which may be be more easily reviewed using `git diff --color-moved` (https://blog.github.com/2018-04-05-git-217-released/).
The motivation for this is to support BIP 157 by indexing block filters.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/bitcoin/bitcoin/13243)
<!-- Reviewable:end -->
Tree-SHA512: 0857f04df2aa920178dab2eb8e57984d8eb4d5010deca9971190358479e05b6672ccca2a08af0a7ac9fe02afb947be84cf35a3693204d0667263c6add2959cbf
ca1a093127c11bb2aea10bf96c38dbfb40f8d170 Add regression test: Don't assert(...) with side effects (practicalswift)
4c3c9c38699360f93d3c52a01a90ff15ee5e1a62 Don't assert(...) with side effects (practicalswift)
Pull request description:
Don't `assert(...)` with side effects.
From the developer notes:
> **Assertions should not have side-effects**
>
> Rationale: Even though the source code is set to refuse to compile with assertions disabled, having side-effects in assertions is unexpected and makes the code harder to understand
These assertions were introduced quite recently (in #14069 which was merged two days ago) and since this is a recurring thing (see #13534 – "Don't assert(foo()) where foo() has side effects" from May) I added a simple regression test for the most obvious common side effect.
Tree-SHA512: be65db9d8d5d0f5752152ba73fe3fbb0531880f156d3cd7dfdf1752709979b63214e46ae64b1adbe1e09fa121278f4087f4ae49bff16cf8f5aec16ea6bde3650
faf4a9b674 qa: Stop txindex thread before calling destructor (MarcoFalke)
Pull request description:
Same as #13894, but for the tests.
Tree-SHA512: a21d9f8ad8dc9703217d1808cb14bd969903c364fe30bbdc0dd2df170ddc0cbaba98b0bde28bc21ff1319222aaf6cb4f1b2c45cd6b236fe3c645a92eab6bacba
737670c036 Use assert when running from multithreaded code as BOOST_CHECK_* are not thread safe (Jesse Cohen)
Pull request description:
Resolves thread sanitizer failure @MarcoFalke found in #14058
Tree-SHA512: 24d86c2cdae21fee029ee4b06f633de4b3e655d3371d97f09db6fd3f24b29388a78110996712249c49e7fefa7bbc3d3c405d8b480382174831fe2f9a042a557e
ddddce0e46e73d4ca369f2ce9696231cc579e1f9 util: Replace boost::signals2 with std::function (MarcoFalke)
Pull request description:
This removes the `#include <boost/signals2/signal.hpp>` from `util.h` (hopefully speeding up the build time and reducing the memory usage further after #13634)
The whole translation interface is replaced by a function `G_TRANSLATION_FUN` that is set to nullptr in units that don't need translation. (Thus only set in the gui)
Tree-SHA512: 087c717358bbed8bdb409463e225239d667f1ced381abb10e7cd31a41dcdd2cebe20b43c2ee86f0f8e55d53301f75e963f07421a99a7ff4c0cad2c6a375c5ab1
# Conflicts:
# src/bench/bench_dash.cpp
# src/qt/dash.cpp
# src/qt/splashscreen.cpp
# src/qt/transactiontablemodel.cpp
# src/test/test_dash.cpp
# src/util/system.h
# src/wallet/coinselection.cpp
6ad0328f1c Don't assert(foo()) where foo has side effects (practicalswift)
Pull request description:
Don't `assert(foo())` where `foo` has side effects.
From `assert(3)`:
> If the macro `NDEBUG` is defined at the moment `<assert.h>` was last included, the macro `assert()` generates no code, and hence does nothing at all.
Bitcoin currently cannot be compiled without assertions, but we shouldn't rely on that.
Tree-SHA512: 28cff0c6d1c2fb612ca58c9c94142ed01c5cfd0a2fecb8e59cdb6c270374b215d952ed3491d921d84dc1b439fa49da4f0e75e080f6adcbc6b0e08be14e54c170
# Conflicts:
# src/bench/block_assemble.cpp
# src/bench/checkblock.cpp
# src/script/sign.cpp
29ed2d64f6 Improve CAmount tests (Hennadii Stepanov)
Pull request description:
This provides:
- more `MoneyRange` tests;
- new `CFeeRate` constructor tests with zero byte size;
- explicit using of the `CAmount` type.
Tree-SHA512: ca0ad6ccb37909a2a5c11034dc07b316a84c32fb40c6f8b6cfc28ebec72a1de157f31d22e767ae80d70ed06d7296f23870cc5ed0689f34a754ae763d50e23d43
b6718e373e tests: Use MakeUnique to construct objects owned by unique_ptrs (practicalswift)
Pull request description:
A subset of #14211 ("Use MakeUnique to construct objects owned by unique_ptrs") as suggested by @MarcoFalke in https://github.com/bitcoin/bitcoin/pull/14211#issuecomment-423324019.
Use `MakeUnique` to construct objects owned by `unique_ptr`s.
Rationale:
* `MakeUnique` ensures exception safety in complex expressions.
* `MakeUnique` gives a more concise statement of the construction.
Tree-SHA512: 1228ae6ce7beb178d79142c4e936b728178ccaa8aa35c6d8feeb33d1a667abfdd010c59996a9d833594611e913877ce5794e75953d11d9b1fdbac04aa491d9cf
ef0b01217a tests: Make updatecoins_simulation_test deterministic (practicalswift)
Pull request description:
Make test `updatecoins_simulation_test` deterministic.
Can be verified using `contrib/test_deterministic_coverage.sh` introduced in #15296.
Related:
* #15296: "tests: Add script checking for deterministic line coverage in unit tests"
* #15324: "test: Make bloom tests deterministic"
* #14343: "coverage reports non-deterministic"
Tree-SHA512: 3466e28a42dd3735effb8542044d88e8350a470729d4a4f02abce9d6367de6568d698131469ba154d3dc76d448bacb360b7aefd066bb5b91408c0be375dd3ecb
6dc4593db1ccfb8745b2daa42f457981ae08dba9 IsReachable is the inverse of IsLimited (DRY). Includes unit tests (marcaiaf)
Pull request description:
IsReachable is the inverse of IsLimited, but the implementation is duplicated (DRY)
- Changed the implementation accordingly.
- Added unit tests to document behavior and relationship
- My modification in net.cpp applies only to IsReachable.
- Applied clang-format-diffpy
Created new pull request to avoid the mess with:
https://github.com/bitcoin/bitcoin/pull/15044
Checked with supposedly conflicting PRs mentioned in the old PR. No conflicts with the specific changes in this PR.
Tree-SHA512: b132dec6cc2c788ebe4f63f228d78f441614e156743b17adebc990de0180a5872874d2724c86eeaa470b4521918bd137b0e33ebcaae77c5efc1f0d56104f6c87
6b25f29a91 Use std::vector API for construction of test data. (Daniel Kraft)
Pull request description:
For constructing test scripts, use `std::vector` and, in particular, `std::vector::insert` to insert 20 zero bytes rather than listing the full array of bytes explicitly. This makes the code easier to read and makes it immediately obvious what the structure of the data is, without having to count the zeros to understand it.
Of course, that is a matter of taste - so if you disagree that the change makes the code easier to read, let me know.
This has been split out of #14752.
Tree-SHA512: af82d447f0077259049f1da2d6f86a6c29723c6e17bd342e9a9ecf37b13bddff40643af95c8b3a3260765a5591713d31ca8a45a5a0c20a12c139aee53ea150da
b81560029 Remove CombineSignatures and replace tests (Andrew Chow)
ed94c8b55 Replace CombineSignatures with ProduceSignature (Andrew Chow)
0422beb9b Make SignatureData able to store signatures and scripts (Andrew Chow)
b6edb4f5e Inline Sign1 and SignN (Andrew Chow)
Pull request description:
Currently CombineSignatures is used to create the final scriptSig or an input. However ProduceSignature is capable of doing this itself. Using both CombineSignatures and ProduceSignature results in code duplication which is unnecessary.
To move the scriptSig construction to ProduceSignatures, the SignatureData class contains two maps to hold pubkeys mapped to signatures, and script ids mapped to scripts. DataFromTransaction is extended to be able to extract signatures, their public keys, and scripts from existing ScriptSigs.
The SignaureData are then passed down to SignStep which can use the aforementioned maps to get the signatures, pubkeys, and scripts that it needs, falling back to the actual SigningProvider and SignatureCreator if the data are not available in the SignatureData.
Additionally, Sign1 and SignN have been removed and their functionality inlined into SignStep since Sign1 is really just a wrapper around CreateSig.
Since ProduceSignature can produce the final scriptSig or scriptWitness by using SignatureData which has extracted data from the transaction, CombineSignatures is unnecessary as ProduceSignature is able to replicate all of CombineSignatures' functionality.
This also furthers BIP 174 support and begins moving towards a BIP 174 style backend.
The tests have also been updated to use the new combining methodology.
Tree-SHA512: 78cd58a4ebe37f79229bd5eee2958a0bb45cd7f36d0e993eee13ff685b3665dd76ef2dfd5f47d34678995bb587f5594100ee5f6c09b1c69ee96d3684d470d01e
bb582a59c Add P2WSH destination helper and use it instead of manual hashing (Pieter Wuille)
eaba1c111 Add additional unit tests for invalid IsMine combinations (Pieter Wuille)
e6b9730c4 Do not expose invalidity from IsMine (Pieter Wuille)
Pull request description:
This improves the handling of INVALID in IsMine:
* Extra INVALID conditions were added to `IsMine` (following https://github.com/bitcoin/bitcoin/pull/13142/files#r185349057), but these were untested. Add unit tests for them.
* In https://github.com/bitcoin/bitcoin/pull/13142#issuecomment-386396975 it was suggested to merge `isInvalid` into the return status. This PR takes a different approach, and removes the `isInvalid` entirely. It was only ever used inside tests, as normal users of IsMine don't care about the reason for non-mine-ness, only whether it is or not. As the unit tests are extensive enough, it seems sufficient to have a black box text (with tests for both compressed and uncompressed keys).
Some addition code simplification is done as well.
Tree-SHA512: 3267f8846f3fa4e994f57504b155b0e1bbdf13808c4c04dab7c6886c2c0b88716169cee9c5b350513297e0ca2a00812e3401acf30ac9cde5d892f9fb59ad7fef
faf52f953b47aac6a39892b037eaa3f08d46b655 tests: Drop variadic macro (MarcoFalke)
Pull request description:
The C++11 constructor of `std::vector` that takes an initializer list, is not `explicit`. Thus, the macro is not required and can be dropped.
Hopefully fixes#13456
Tree-SHA512: 4095ed205f88138a7cd5b14790cc426899966f622a924a9b3f7de646a0d801a48ffb8921da760f1f93d5481298477c8a64dbec291381bb9aa77b075bdd2659f2
2acd1d6716959b99e751cf85a7c47aaa383e937f Drop uint 256 not operator (Ben Woosley)
Pull request description:
All the other operators are integer or bitwise operations, and this is unused
apart from tests.
Note attempting to call `!` on `arith_uint256` results in a build error after this change:
```
test/arith_uint256_tests.cpp:201:17: error: invalid argument type 'const arith_uint256' to unary expression
BOOST_CHECK(!ZeroL);
```
Tree-SHA512: 5791b643f426dac9829e9499d678786f1ad294edb2d840879252a1b642bda55941632114f64048660a5991a984aeba49eeb5dfe64ba0a6275cbe7b1c049d7095
ebebedce20 speed up of tx_validationcache_tests by reusing of CTransaction. (lucash.dev@gmail.com)
Pull request description:
The code was converting CMutableTransaction to CTransaction multiple times, which implies recalculating the hash multiple times. This commit fixes this by reusing a single CTransaction.
Run-time results:
```
Before: 6.7s
After: 5.5s
--------------
Saved: 1.2s
```
This PR was split from #13050. Also, see #10026.
Tree-SHA512: 61fb81972a08299085a7d3d0060485b265aefc7a4f82ab548e5f94371c8643cfb97bf0ef34f4e1211bf853d0217fa1c3338e4117f36fda1b37d203f690e86d60
903055730b Test gArgs erroring on unknown args (Andrew Chow)
4f8704d57f Give an error and exit if there are unknown parameters (Andrew Chow)
174f7c8080 Use a struct for arguments and nested map for categories (Andrew Chow)
Pull request description:
Following #13190, gArgs is aware of all of the command line arguments. This PR has gArgs check whether the arguments provided are actually valid arguments. When an unknown argument is encountered, an error is printed to stderr and the program exist.
Since gArgs is used for everything that has command line arguments, `bitcoind`, `bitcoin-cli`, `bitcoin-qt`, `bitcoin-tx`, and `bench_bitcoin` are all effected by this change and all now have the same argument checking behavior.
Closes#1044
Tree-SHA512: 388201319a7d6493204bb5433da47e8e6c8266882e809f6df45f86d925f1f320f2fd13edb3e57ffc6a37415dfdfc689f83929452bca224229783accb367032e7
fac1223a568fa1ad6dd602350598eed278d115e8 Cache witness hash in CTransaction (MarcoFalke)
faab55fbb17f2ea5080bf02bc59eeef5ca746f07 Make CMutableTransaction constructor explicit (MarcoFalke)
Pull request description:
This speeds up:
* compactblocks (v2)
* ATMP
* validation and miner (via `BlockWitnessMerkleRoot`)
* sigcache (see also unrelated #13204)
* rpc and rest (nice, but irrelevant)
This presumably slows down rescan, which uses a `CTransaction` and its `GetHash`, but never uses the `GetWitnessHash`. The slow down is proportional to the number of witness transactions in the rescan window. I.e. early in the chain there should be no measurable slow down. Later in the chain, there should be a slow down, but acceptable given the speedups in the modules mentioned above.
Tree-SHA512: 443e86acfcceb5af2163e68840c581d44159af3fd1fce266cab3504b29fcd74c50812b69a00d41582e7e1c5ea292f420ce5e892cdfab691da9c24ed1c44536c7
6249021d1 [docs] Add release notes for HD master key -> HD seed rename (John Newbery)
79053a5f2 [rpc] [wallet] Add 'hdmasterkeyid' alias return values. (John Newbery)
c75c35141 [refactor] manually change remaining instances of master key to seed. (John Newbery)
131d4450b scripted-diff: Rename master key to seed (John Newbery)
Pull request description:
Addresses #12084 and #8684
This renames a couple of functions and members (no functional changes, expect log prints):
- Rename CKey::SetMaster to CKey::SetSeed
- Rename CHDChain::masterKeyId to CHDChain::seedID
- Rename CHDChain::hdMasterKeyID to CHDChain::hdSeedID
- Rename CWallet::GenerateNewHDMasterKey to CWallet::GenerateNewHDSeed
- Rename CWallet::SetHDMasterKey to CWallet::SetHDSeed
As well it introduces a tiny API change:
- RPC API change: Rename "hdmasterkeyid" to "hdseedid", rename "hdmaster" in wallet-dump output to "hdseed"
Fixes also a bug:
- Bugfix: use "s" instead of the incorrect "m" for the seed-key hd-keypath key metadata
Tree-SHA512: c913252636f213135a3b64df5de5d21844fb9c2d646567c1aad0ec65745188587de26119de99492c67e559bd49fdd9606b54276f00dddb84301785beba58f281
d48f664440e7bb3ff7a90b6d706a3ac2cfaec95a tests: Fix fs_tests for unknown locales (Daki Carnhof)
Pull request description:
Fix by removing "L" as suggested by meeDamian in
https://github.com/bitcoin/bitcoin/issues/14948#issuecomment-522355441
```
# all in .../bitcoin/src/test
$ uname -m
x86_64
$ export LC_ALL=randomnonexistentlocale
$ ./test_bitcoin
Running 369 test cases...
unknown location(0): fatal error: in "fs_tests/fsbridge_fstream": boost::system::system_error: boost::filesystem::path codecvt to string: error
test/fs_tests.cpp(13): last checkpoint: "fsbridge_fstream" test entry
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
```
After the patch is applied, the same test under the same conditions runs fine.
```
$ export LC_ALL=randomnonexistentlocale
$ ./test_bitcoin
Running 369 test cases...
*** No errors detected
```
Co-Authored-By: bugs@meedamian.com
ACKs for top commit:
laanwj:
ACK d48f664440e7bb3ff7a90b6d706a3ac2cfaec95a
Tree-SHA512: a9910252b8ce6a05cab5530874549c2999ca2c28e835fc18aa8e5468fb417bd7d245864ec71d9233dd53e02940a9f0691b247430257f27eb0d7c20745d1c846d
1cc58978b7 tests: Fix accidental trunction from int to bool (practicalswift)
Pull request description:
Fix accidental trunction from `int` to `bool`.
Context: https://github.com/bitcoin/bitcoin/pull/14086#issuecomment-416610313
Tree-SHA512: 72d209f892e580afa9c295174c206ea5ba764ff9e03613cd9bc57fd0d7118e895ee44d96db90930a29c0b4de7f51dc00101a1b32ba6b46576d34e089ff5482ba
60ebc7da4c trivial: Mark overrides as such. (Daniel Kraft)
Pull request description:
This trivial change adds the `override` keyword to some methods that override virtual base class / interface methods. This ensures that any future changes to the interface's method signatures which are not correctly mirrored in the subclasses will break at compile time with a clear error message, rather than at runtime.
Tree-SHA512: cc1bfa5f03b5e29d20e3eab07b0b5fa2f77b47f79e08263dbff43e4f463e9dd8f4f537e2c8c9b6cb3663220dcf40cfd77723cd9fcbd623c9efc90a4cd44facfc
dd435ad Add unit tests for signals generated by ProcessNewBlock() (Jesse Cohen)
a3ae8e6 Fix concurrency-related bugs in ActivateBestChain (Jesse Cohen)
ecc3c4a Do not unlock cs_main in ABC unless we've actually made progress. (Matt Corallo)
Pull request description:
Originally this PR was just to add tests around concurrency in block validation - those tests seem to have uncovered another bug in ActivateBestChain - this now fixes that bug and adds tests.
ActivateBestChain (invoked after a new block is validated) proceeds in steps - acquiring and releasing cs_main while incrementally disconnecting and connecting blocks to sync to the most work chain known (FindMostWorkChain()). Every time cs_main is released the result of FindMostWorkChain() can change - but currently that value is cached across acquisitions of cs_main and only refreshed when an invalid chain is explored. It needs to be refreshed every time cs_main is reacquired. The test added in 6094ce7304 will occasionally fail without the commit fixing this issue 26bfdbaddb
Original description below
--
After a bug discovered where UpdatedBlockTip() notifications could be triggered out of order (#12978), these unit tests check certain invariants about these signals.
The scheduler test asserts that a SingleThreadedSchedulerClient processes callbacks fully and sequentially.
The block validation test generates a random chain and calls ProcessNewBlock from multiple threads at random and in parallel. ValidationInterface callbacks verify that the ordering of BlockConnected BlockDisconnected and UpdatedBlockTip events occur as expected.
Tree-SHA512: 4102423a03d2ea28580c7a70add8a6bdb22ef9e33b107c3aadef80d5af02644cdfaae516c44933924717599c81701e0b96fbf9cf38696e9e41372401a5ee1f3c
364bae5 qa: Pad scriptPubKeys to get minimum sized txs (MarcoFalke)
7485488 Policy to reject extremely small transactions (Johnson Lau)
0f8719b Add transaction tests for constant scriptCode (Johnson Lau)
9dabfe4 Add constant scriptCode policy in non-segwit scripts (Johnson Lau)
Pull request description:
This disables `OP_CODESEPARATOR` in non-segwit scripts (even in an unexecuted branch), and makes a positive `FindAndDelete` result invalid. This ensures that the `scriptCode` serialized in `SignatureHash` is always the same as the script passing to the `EvalScript`.
Tree-SHA512: a0552cb920294d130251c48053fa2ff1fbdd26332e62b52147d918837852750f0ce35ce2cd1cbdb86588943312f8154ccb4925e850dbb7c2254bc353070cd5f8
7d0f80b Use anonymous namespace instead of static functions (Pieter Wuille)
b61fb71 Mention removal of bare multisig IsMine in release notes (Pieter Wuille)
9c2a8b8 Do not treat bare multisig as IsMine (Pieter Wuille)
08f3228 Optimization: only test for witness scripts at top level (Pieter Wuille)
3619735 Track difference between scriptPubKey and P2SH execution in IsMine (Pieter Wuille)
ac6ec62 Switch to a private version of SigVersion inside IsMine (Pieter Wuille)
19fc973 Do not expose SigVersion argument to IsMine (Pieter Wuille)
fb1dfbb Remove unused IsMine overload (Pieter Wuille)
952d821 Make CScript -> CScriptID conversion explicit (Pieter Wuille)
Pull request description:
Currently our wallet code will treat bare multisig outputs (meaning scriptPubKeys with multiple public keys + `OP_CHECKMULTISIG` operator in it) as ours without the user asking for it, as long as all private keys in it are in our wallet.
This is a pointless feature. As it only works when all private keys are in one place, it's useless compared to single key outputs (P2PK, P2PKH, P2WPKH, P2SH-P2WPKH), and worse in terms of space, cost, UTXO size, and ability to test (due to lack of address format for them).
Furthermore, they are problematic in that producing a list of all `scriptPubKeys` we accept is not tractable (it involves all combinations of all public keys that are ours). In further wallet changes I'd like to move to a model where all scriptPubKeys that are treated as ours are explicit, rather than defined by whatever keys we have. The current behavior of the wallet is very hard to model in such a design, so I'd like to get rid of it.
I think there are two options:
* Remove it entirely (do not ever accept bare multisig outputs as ours, unless watched)
* Only accept bare multisig outputs in situations where the P2SH version of that output would also be acceptable
This PR implements the first option. The second option was explored in #12874.
Tree-SHA512: 917ed45b3cac864cee53e27f9a3e900390c576277fbd6751b1250becea04d692b3b426fa09065a3399931013bd579c4f3dbeeb29d51d19ed0c64da75d430ad9a
67d6ee1 remove redundant tests in p2p-segwit.py (Johnson Lau)
9260085 test segwit uncompressed key fixes (Johnson Lau)
248f3a7 Fix ismine and addwitnessaddress: no uncompressed keys in segwit (Pieter Wuille)
b811124 [qa] Add tests for uncompressed pubkeys in segwit (Suhas Daftuar)
9f0397a Make test framework produce lowS signatures (Johnson Lau)
4c0c25a Require compressed keys in segwit as policy and disable signing with uncompressed keys for segwit scripts (Johnson Lau)
3ade2f6 Add standard limits for P2WSH with tests (Johnson Lau)
f9ee0f37c28f604bc82dab502ce229c66ef5b3b9 Add comments to CustomUintFormatter (Pieter Wuille)
4eb5643e3538863c9d2ff261f49a9a1b248de243 Convert everything except wallet/qt to new serialization (Pieter Wuille)
2b1f85e8c52c8bc5a17eae4c809eaf61d724af98 Convert blockencodings_tests to new serialization (Pieter Wuille)
73747afbbeb013669faf4c4d2c0903cec4526fb0 Convert merkleblock to new serialization (Pieter Wuille)
d06fedd1bc26bf5bf2b203d4445aeaebccca780e Add SER_READ and SER_WRITE for read/write-dependent statements (Russell Yanofsky)
6f9a1e5ad0a270d3b5a715f3e3ea0911193bf244 Extend CustomUintFormatter to support enums (Russell Yanofsky)
769ee5fa0011ae658770586442715452a656559d Merge BigEndian functionality into CustomUintFormatter (Pieter Wuille)
Pull request description:
The next step of changes from #10785.
This:
* Adds support for enum serialization to `CustomUintFormatter`, used in `CAddress` for service flags.
* Merges `BigEndian` into `CustomUintFormatter`, used in `CNetAddr` for port numbers.
* Converts everything (except wallet and gui) to use the new serialization framework.
ACKs for top commit:
MarcoFalke:
re-ACK f9ee0f37c2, only change is new documentation commit for CustomUintFormatter 📂
ryanofsky:
Code review ACK f9ee0f37c28f604bc82dab502ce229c66ef5b3b9. Just new commit adding comment since last review
jonatack:
Code review re-ACK f9ee0f37c28f604bc82dab502ce229c6 only change since last review is an additional commit adding Doxygen documentation for `CustomUintFormatter`.
Tree-SHA512: e7a0a36afae592d5a4ff8c81ae04d858ac409388e361f2bc197d9a78abca45134218497ab2dfd6d031e0cce0ca586cf857077b7c6ce17fccf67e2d367c1b6cd4
60f61f9 Tighten up bech32::Decode(); add tests. (murrayn)
Pull request description:
Just a few minor optimizations to bech32::Decode():
1) optimize the order and logic of the conditionals
2) get rid of subsequent '(c < 33 || c > 126)' check which is redundant (already performed above)
3) add a couple more bech32 tests (mixed-case)
Tree-SHA512: e41af834c8f6b7d34c22c28b724df42c60f72e00df616e70a12efbc4271d15d80627fe1bc36845caf29f615c238499a566298a863cbe119fef457287231053c8
159c32d1f1 Add assertion to guide static analyzers. Clang Static Analyzer needs this guidance. (practicalswift)
fd447a6efe Fix dead stores. Values were stored but never read. Limit scope. (practicalswift)
Pull request description:
Fix Clang Static Analyzer warnings reported by @kallewoof in #12961:
* Fix dead stores. Values were stored but never read.
* Add assertion to guide static analyzers. See #12961 for details.
Tree-SHA512: 83dbec821f45217637316bee978e7543f2d2caeb7f7b0b3aec107fede0fff8baa756da8f6b761ae0d38537740839ac9752f6689109c38a4b05c0c041aaa3a1fb
c3f34d06be Make it clear which functions that are intended to be translation unit local (practicalswift)
Pull request description:
Make it clear which functions that are intended to be translation unit local.
Do not share functions that are meant to be translation unit local with other translation units. Use internal linkage for those consistently.
Tree-SHA512: 05eebd233d5cfbf6116724eec3a99b465bf534ca220f2b6f5e56341a7da41387454d3cb6ceadd8ab6714a5df94069e4ad0dcab8801ccc7e8949be7199a19fb53
8c2d695c4a util: Store debug log file path in BCLog::Logger member. (Jim Posen)
8e7b961388 scripted-diff: Rename BCLog::Logger member variables. (Jim Posen)
1eac317f25 util: Refactor GetLogCategory. (Jim Posen)
3316a9ebb6 util: Encapsulate logCategories within BCLog::Logger. (Jim Posen)
6a6d764ca5 util: Move debug file management functions into Logger. (Jim Posen)
f55f4fcf05 util: Establish global logger object. (Jim Posen)
Pull request description:
This is purely a refactor with no behavior changes.
This creates a new class `BCLog::Logger` to encapsulate all global logging configuration and state.
Tree-SHA512: b34811f54a53b7375d7b6f84925453c6f2419d21179379ee28b3843d0f4ff8e22020de84a5e783453ea927e9074e32de8ecd05a6fa50d7bb05502001aaed8e53
9b2704777c [doc] Include txindex changes in the release notes. (Jim Posen)
ed77dd6b30 [test] Simple unit test for TxIndex. (Jim Posen)
6d772a3d44 [rpc] Public interfaces to GetTransaction block until synced. (Jim Posen)
a03f804f2a [index] Move disk IO logic from GetTransaction to TxIndex::FindTx. (Jim Posen)
e0a3b80033 [validation] Replace tx index code in validation code with TxIndex. (Jim Posen)
8181db88f6 [init] Initialize and start TxIndex in init code. (Jim Posen)
f90c3a62f5 [index] TxIndex method to wait until caught up. (Jim Posen)
70d510d93c [index] Allow TxIndex sync thread to be interrupted. (Jim Posen)
94b4f8bbb9 [index] TxIndex initial sync thread. (Jim Posen)
34d68bf3a3 [index] Create new TxIndex class. (Jim Posen)
c88bcec93f [db] Migration for txindex data to new, separate database. (Jim Posen)
0cb8303241 [db] Create separate database for txindex. (Jim Posen)
Pull request description:
I'm re-opening #11857 as a new pull request because the last one stopped loading for people
-------------------------------
This refactors the tx index code to be in it's own class and get built concurrently with validation code. The main benefit is decoupling and moving the txindex into a separate DB. The primary motivation is to lay the groundwork for other indexers that might be desired (such as the [compact filters](https://github.com/bitcoin/bips/pull/636)). The basic idea is that the TxIndex spins up its own thread, which first syncs the txindex to the current block index, then once in sync the BlockConnected ValidationInterface hook writes new blocks.
### DB changes
At the suggestion of some other developers, the txindex has been split out into a separate database. A data migration runs at startup on any nodes with a legacy txindex. Currently the migration blocks node initialization until complete.
### Open questions
- Should the migration of txindex data from the old DB to the new DB block in init or should it happen in a background thread? The downside to backgrounding it is that `getrawtransaction` would return an error message saying the txindex is syncing while the migration is running.
### Impact
In a sample size n=1 test where I synced nodes from scratch, the average time [Index writing](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L1903) was 3.36ms in master and 1.72ms in this branch. The average time between `UpdateTip` log lines for sequential blocks between 400,000 and IBD end on mainnet was 0.297204s in master and 0.286134s in this branch. Most likely this is just variance in IBD times, but I can try with some more trials if people want.
Tree-SHA512: 451fd7d95df89dfafceaa723cdf0f7b137615b531cf5c5035cfb54e9ccc2026cec5ac85edbcf71b7f4e2f102e36e9202b8b3a667e1504a9e1a9976ab1f0079c4
Comment from 6f7b52ac63a71d2706022ca58d69a1a622e0fa37: "The fix for CPubKey is a part of `#13557: BIP 174 PSBT Serializations and RPCs` which wasn't backported yet"
* evo: Remove all protx-es that refer to a ProRegTx removed from mempool
* tests: Check that removal of ProRegTx causes removal of other protx-es that refer to it
* evo: Consider tx itself a collateral in mempool maps when payload collateral hash is null
* tests: Should not allow a ProRegTx which uses another ProRegTx as an external collateral to enter mempool
* tests: Add more unit tests for bls/bls.cpp
* Make bls_key_agg_tests a bit more readable
* Compare pubkeys, not their string representations
* Use GetRandHash
* Clarify sig manipulations in bls_sig_agg_sub_tests
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
fae58eca93 tests: Avoid copies of CTransaction (MarcoFalke)
Pull request description:
Avoid the copy (or move) constructor of `CTransaction` in test code, whereever a simple reference can be used instead.
Tree-SHA512: 8ef2077a277d6182996f4671722fdc01a90909ae7431c1e52604aab8ed028910615028caf9b4cb07a9b15fdc04939dea2209cc3189dde7d38271256d9fe1076c
1527015 Avoid std::locale/imbue in DateTimeStrFormat (Pieter Wuille)
Pull request description:
And replace them with just hardcoded ISO8601 strings and `gmtime_r`.
Pointed out by @laanwj here: https://github.com/bitcoin/bitcoin/pull/12970#issuecomment-380962488
Tree-SHA512: a459758b42ca56f8462115aefe8e6377c1319fce509ea64dbb767f3f087c9b848335954cb684e5896c38008847684045505a3e1559fb3e83b8e80e10b003d1e7
c55aa4f test: Fix sign for expected values (Karl-Johan Alm)
Pull request description:
A number of `BOOST_CHECK_EQUAL` calls would result in warnings about signs.
This PR fixes signedness for all expectation values, sometimes resulting in `int` → `unsigned int`. No other code changes besides adding/removing `U` to/from values.
Running `make &> make_output_...` on master versus on this PR:
```
$ wc make_output_*
1464 5925 90357 make_output_master
613 1469 28370 make_output_signfixed
```
More than halves the output lines from compiling.
Tree-SHA512: b06c9fb81704fd32a6a61fe7b2ceb5f1bb381e9873d79e13d7e4d26bbd9b67c9725a84e6fb2903bcda775aea2a792e544b0799d36735c19f5d1c7225e8c6d14e
* evo: Pass CCoinsViewCache instead of relying on pcoinsTip
This ensures that we are on the same page with ConnectBlock etc.
* evo: Process special txes before updating UTXOs
This ensures consistency between the way we do it in blocks and in mempool
* test: Verify db consistency after MN collateral is spent via ProTx that updates the same MN
* Make stuff const
* more constness
Co-authored-by: pasta <pasta@dashboost.org>
fa5ed4f8d2 refactor: Avoid locking tx pool cs thrice (MarcoFalke)
Pull request description:
`addUnchecked` is (outside the tests) only called by ATMP, which already takes the tx pool read lock. So locking it twice more in both `addUnchecked` methods seems redundant.
Similarly `CalculateMemPoolAncestors` is (beside once in the wallet) only called in contexts, where the tx pool lock is already taken. So remove the lock there as well.
Tree-SHA512: fcf603b570da0fc529fe6db8add218663eae52845510732bee0d4611263d2429d3d3c9c8ae68493d67287d13504500ed51905ccbe711eb15a0af3b019edad543
