43e7d576f5 doc: Improve test READMEs (Fabian Jahr)
Pull request description:
General improvements on READMEs for unit tests and functional tests:
- Give unit test readme a headline
- Move general information on `src/test` folder to the top
- Add information on logging and debugging unit tests
- Improve debugging and logging information in functional testing
- Include all available log levels in functional tests
ACKs for top commit:
laanwj:
ACK 43e7d576f5
Tree-SHA512: 22b27644992ba5d99a885cd51b7a474806714396fcea1fd2d6285e41bdf3b28835ad8c81449099e3ee15a63d57b3ab9acb89c425d9855ed1d9b4af21db35ab03
fdb3e8f8b2 Ignore old versionbit activations (Anthony Towns)
Pull request description:
PR 16060 removed the CSV and Segwit BIP9 softfork definitions and hard-coded ('buried') the activation heights. The versionbits code will warn users if an undefined softfork has been signalled in block header versions, and removing the CSV/Segwit definitions caused those warnings to be triggered.
Change the BIP 9 warning code to only check for unknown softforks after the segwit activation height.
ACKs for top commit:
MarcoFalke:
ACK fdb3e8f8b2
ajtowns:
ACK fdb3e8f8b2 for what it's worth
achow101:
ACK fdb3e8f8b2
Sjors:
ACK fdb3e8f8b2. It makes the bit 0 warning go away in mainnet and testnet QT when a new block arrives. I think the code is clear enough.
jonatack:
ACK fdb3e8f8b2
Tree-SHA512: e6fd34e8902f8c7affb28e8951803e47d542710d5f1229000746656a37ee59d754439fc33e36b7eef87544262e5aac374645db91b74cb507e73514003ca7a67f
1a02edb3f2 [RPC] Fix casing in getblockchaininfo to be inline with the rest of the response (Dan Gershony)
Pull request description:
The response in the RPC result `startTime` is camel cased while the rest of the response seems to be lower cased.
If this was intentional please ignore and close this PR.
Note: RPC field case changes might break existing callers
ACKs for top commit:
laanwj:
ACK 1a02edb3f2
Tree-SHA512: 6f0eaf2b4aaf73c9a9bf1fbd4af59af5f95fc012fa88f94e050e6ae273b3ad647f5729df53bfce91e1a925fe4fd7b14818908bb6131a81413a555137d1007d7c
8d841ad492 doc: Remove MSVC update step from translation process (Wladimir J. van der Laan)
Pull request description:
This part of the build system has been removed in #15529 and thus no longer needs to be updated.
ACKs for top commit:
MarcoFalke:
ACK 8d841ad492
sipsorcery:
ACK 8d841ad
Tree-SHA512: f561a6b1da806e8868a265c77725b94fabef60bc7b9d401e3f70c3d859323adc2e204e3d6fbfea4f1ff86e70667f8bd01157411106ea93974921c02d874e0083
The response in the RPC result `starttime` is camel cased while the rest of the response seems to be lower cased.
If this was intentional please ignore this PR.
Note: case might break existing callers
Reflect the change in the test data
Change to snake case
efd2474d17 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.~ efd2474d17
mzumsande:
I had intended to follow up earlier on my last comment, ACK efd2474d17. I reviewed the code, ran tests and did a successful reindex on testnet with this branch.
Tree-SHA512: 695529e0af38bae2af4e0cc2895dda56a71b9059c3de04d32e09c0165a50f6aacee499f2042156ab5eaa6f0349bab6bcca4ef9f6f9ded4e60d4483beab7e4554
fa607c2292 validation: Make GetWitnessCommitmentIndex public (MarcoFalke)
Pull request description:
`GenerateCoinbaseCommitment` is public and can be used in unit tests to update the witness commitment after the list of txs in a block has been changed. However, for it to work, the existing commitment (added by default in `CreateNewBlock`) must be removed (and thus its index must be known).
Make that possible by exposing the `GetWitnessCommitmentIndex` helper function in the header.
ACKs for top commit:
jb55:
ACK fa607c2292
jamesob:
ACK fa607c2292
promag:
ACK fa607c2292.
fanquake:
ACK fa607c2292 - This unblocks work in #15845.
Tree-SHA512: d563aa2c201d5fb4874e506a28f468c37e457cc8a20229c377178af08c22d3be44e19ee6e8e524b6de99236cd5f2c9e39b8009d88c26854aa774737912bd5889
f4beb4996d test: Remove python dead code linter (Wladimir J. van der Laan)
Pull request description:
Primarily I'd like to remove this because it is very imprecise, due to Python's dynamic nature, giving it a large list of false positives that need to be listed as exceptions. See for example #16906.
It's also a frequent source of complaints. I'm doubtful of the usefulness of checking for dead code in a linter in the first place.
Having some dead code in the test framework for a while is not a
disaster.
ACKs for top commit:
sdaftuar:
utACK f4beb4996d
practicalswift:
ACK f4beb4996d -- diff looks correct
jamesob:
ACK f4beb4996d
Tree-SHA512: 329b1555210311d5d15799fd2cb794b3208b0ac4d8a2ffaf4dece1bcc3e0e8b1fe952d5e7a394f94a98919cab579fb579eae7db2a796cc9a1a42ef495dd17507
fa7847d99b gui: Rename address checkbox back to bech32 (MarcoFalke)
Pull request description:
This is the wording that has been used in the previous release, so translations should still exist for it.
Fixes: #16924
ACKs for top commit:
promag:
ACK fa7847d99b.
laanwj:
ACK fa7847d99b
Tree-SHA512: 0ac6c47fe5eb2145b609a30fd3f56052d3e08abe6c67fc74b6d209a55a4df509c52f13eb1c759520a4fa43916ece0e6d4cefef87e061b51114a6582db911944a
fadd76acc2 ci: Remove TRAVIS env vars (MarcoFalke)
fa449b89b5 ci: Set $HOST before setting fallback values (MarcoFalke)
Pull request description:
This shouldn't change anything, except that `$HOST` is now properly set in the `$BASE_OUTDIR`. (Previously it would always use `x86_64-unknown-linux-gnu` in the directory name and now it should use the correct host)
The second commit removes travis environment variables in the `ci` system. Also, shouldn't change any behavior.
ACKs for top commit:
fanquake:
ACK fadd76acc2 - assuming Travis etc is happy.
Tree-SHA512: aafd65bfc039523208b17d1ed886a3311995d984ec56c3de5f837b5a71d985061ee2da7af947f95a56ab101a0666fe7cd99434e196cd1b7ee9c460d156a185f6
6659810e2f test: use named args for sendrawtransaction calls (Jon Atack)
5c1cd78b7e doc: improve rawtransaction code/test docs (Jon Atack)
acc14c5093 test: fix incorrect value in rpc_rawtransaction.py (Jon Atack)
Pull request description:
Follow-up to PR #16521.
- Fix incorrect value in rpc_rawtransaction test as per https://github.com/bitcoin/bitcoin/pull/16521/files#r325842308
- Improve the code docs
- Use named arguments as per https://github.com/bitcoin/bitcoin/pull/16521/files#r310715127
Happy to squash or keep only the first commit if the others are too fixup-y.
ACKs for top commit:
laanwj:
ACK 6659810e2f
Tree-SHA512: bf5258f23802ab3ba3defb8791097e08e63f3e2af21023f832cd270dc88d1fa04349e921d69f9f5fedac5dce5cd3c1cc46b48febbede4bc18dccb8be994565b2
27fcb40fc0 doc: replace outdated OpenSSL comment in test README (fanquake)
Pull request description:
The OpenSSL dependency was removed in #15826.
ACKs for top commit:
laanwj:
ACK 27fcb40fc0
theStack:
ACK 27fcb40fc0
Tree-SHA512: eb7a3b18fefa91e6f27c50fa065d6cc330f7b633ae8ee51145cdeec4df51dea5155f0d1fa91e75f1202adef04e063f3eda12773cd00a093f29f5a5e83c4fda73
Primarily I'd like to remove this because it is very imprecise, due to
Python's dynamic nature, giving it a large list of false positives that
need to be listed as exceptions. See for example #16906.
It's also a frequent source of complaints. I'm doubtful of the
usefulness of checking for dead code in a linter in the first place.
Having some dead code in the test framework for a while is not a
disaster.
dbdc758c27 doc: Improve doxygen readme navigation section (Jon Layton)
c15ac2c0aa doc: Move doxygen intro to file for USE_MDFILE_AS_MANPAGE (Jon Layton)
Pull request description:
With `USE_MDFILE_AS_MAINPAGE`, this moves the introductory Doxygen comment to its own file. This makes `bitcoind.cpp` cleaner.
It also removes the `\mainpage` header text, which was smaller than the section titles, and improves the Navigation section.
ACKs for top commit:
promag:
ACK dbdc758c27.
Tree-SHA512: 9352baad655877437913b74dc8888a71d1cccf55a837657ee2630fde3f427d0f0339155b7ab3d9e63a9edb9d53512d747eafcb11987a7c26c47a6df2eca93351
faa079db92 travis: Disable feature_block in tsan run (MarcoFalke)
Pull request description:
The `feature_block` test causes the travis machine to OOM, when run with the thread sanitizer.
The stderr says:
```
==27237==ERROR: ThreadSanitizer failed to allocate 0xf6000 (1007616) bytes of LargeMmapAllocator (error code: 12)
...
FATAL: ThreadSanitizer CHECK failed: /build/llvm-toolchain-3.8-_PD09B/llvm-toolchain-3.8-3.8/projects/compiler-rt/lib/sanitizer_common/sanitizer_common.cc:183 "((0 && "unable to mmap")) != (0)" (0x0, 0x0)
ERROR: Failed to mmap
```
(from https://travis-ci.org/bitcoin/bitcoin/jobs/588194563#L10505)
Fix this by disabling `feature_block` on travis. Longer term, I'd like to move away from travis, but I'll leave this for a follow-up.
ACKs for top commit:
fanquake:
ACK faa079db92
Tree-SHA512: c0dc2272853aac53f68eb9e110c8500c4a92211ba89d856660bacdf6e959d875477e422b3280b743d85fc8a65e083bf9153911f12039d026e2501f426540dac4
6170ec5d3a Do not query all DNS seed at once (Pieter Wuille)
Pull request description:
Before this PR, when we don't have enough connections after 11 seconds, we proceed to query all DNS seeds in a fixed order, loading responses from all of them.
Change this to to only query three randomly-selected DNS seed. If 11 seconds later we still don't have enough connections, try again with another one, and so on.
This reduces the amount of information DNS seeds can observe about the requesters by spreading the load over all of them.
ACKs for top commit:
Sjors:
ACK 6170ec5d3
sdaftuar:
ACK 6170ec5d3a
jonasschnelli:
utACK 6170ec5d3a - I think the risk of a single seeder codebase is orthogonal to this PR. Such risks could also be interpreted differently (diversity could also increase the risk based on the threat model).
fanquake:
ACK 6170ec5d3a - Agree with the reasoning behind the change. Did some testing with and without `-forcednsseed` and/or a `peers.dat` and monitored the DNS activity.
Tree-SHA512: 33f6be5f924a85d312303ce272aa8f8d5e04cb616b4b492be98832e3ff37558d13d2b16ede68644ad399aff2bf5ff0ad33844e55eb40b7f8e3fddf9ae43add57
fa69588537 test: Make PORT_MIN in test runner configurable (MarcoFalke)
Pull request description:
This is needed when some ports in the port range are used by other processes. Note that simply assigning the ports dynamically does not work:
* We spin up several nodes per test (each node gets its own port)
* We run several tests in parallel
So to avoid nodes from different tests colliding on ports, the port assignment must be deterministic (can not be dynamic).
Fixes: #10869
ACKs for top commit:
practicalswift:
ACK fa69588537 -- diff looks correct
promag:
ACK fa69588537.
Tree-SHA512: e79adb015e7de79064e2d14336c38bc9672bd779ad6c52917721897e73f617c39d32c068a369c26670002a6c4ab95a71ef3a6878ebdd9710e02f410e2f7bcd14
bb99c4e684 build: update boost macros to latest upstream (fanquake)
Pull request description:
Fixes: #16803
I opened an [upstream PR](https://github.com/autoconf-archive/autoconf-archive/pull/197) to improve the Boost error reporting, so pull the latest macros.
ACKs for top commit:
laanwj:
Code review ACK bb99c4e684
jonatack:
Sanity check ACK bb99c4e684, light code read, built and ran tests on Debian 4.19.37-5+deb10u2 (2019-08-08) x86_64 GNU/Linux. Only tested the happy path.
Tree-SHA512: 34704ed623ac0085215fd874a23fde8f6e39a69fa20d78472b0c4d2306dc101c0571fa26c4c8821600746b94daaaf05faf6d15546899d588081c26357d29ec46
fa2e038691 test: Fix extra_args in wallet_import_rescan.py (MarcoFalke)
Pull request description:
Bug introduced by me (🤦♂️) in fa25668e1c
For reference:
```
>>> a = [[]]*2
>>> a[0] += ['ONE']
>>> a
[['ONE'], ['ONE']]
>>> a = [[] for _ in range(2)]
>>> a[0] += ['ONE']
>>> a
[['ONE'], []]
ACKs for top commit:
theStack:
utACK fa2e038
Tree-SHA512: 7d75a0d06233d013d62198ea95793612242254d5d90f393d01b2beef5abc78d6e85c796532311638f16cfed3b66a7ae41a108c0fe6f0f5d7f6616b042c670df7
96299a9d6c Test: Move common function assert_approx() into util.py (fridokus)
Pull request description:
To reduce code duplication, move `assert_approx` into common framework `util.py`.
`assert_approx()` is used in two functional tests.
ACKs for top commit:
theStack:
ACK 96299a9
practicalswift:
ACK 96299a9d6c -- DRY is good and diff looks correct
fanquake:
ACK 96299a9d6c - thanks for contributing 🍻
Tree-SHA512: 8e9d397222c49536c7b3d6d0756cc5af17113e5af8707ac48a500fff1811167fb2e03f3c0445b0b9e80f34935f4d57cfb935c4790f6f5463a32a67df5f736939
14c6a2de1a [doc] update brew instruction for doxygen (Sjors Provoost)
Pull request description:
I noticed while testing #16912 that `brew install doxygen --with-graphviz` no long works. Instead you need to use `brew install graphviz doxygen`.
ACKs for top commit:
fanquake:
ACK 14c6a2de1a - tested a `make docs` on macOS with and without `graphviz` (`dot`) available.
Tree-SHA512: 2682568e558c16e9e0a657421c449b74cc14a89771844c1c88623fb75b07b89afb63c45a919eb7b9c3dba9bdfaef21489b5f7ea45a08d8d5da18614657c19e47
fa8d65f071 doc: Fix doxygen comment for SignTransaction in rpc/rawtransaction_util (MarcoFalke)
Pull request description:
The param `coins` to `SignTransaction` is final and can thus not be extended (as suggested by the doc).
ACKs for top commit:
practicalswift:
ACK fa8d65f071 -- const correctness is good and diff looks correct
fanquake:
ACK fa8d65f071
Tree-SHA512: 041e159f2c3cf96e296173c31f3e5f35bbc7711cc888aa4bf08aaa8c65c95ee7f7672f65396690a9af45795a618eea0fadde7fb02d29ec85f1b4df5e6d9e0c7a
3cf36736e5 refactoring: move ReplayBlocks under CChainState (James O'Beirne)
bcf73d3b84 refactoring: move LoadChainTip to CChainState method (James O'Beirne)
f5809d5b13 doc: fix CChainState::ActivateBestChain doc (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
Move more chainstate-related functionality to methods on CChainState. Nothing too interesting here, but needed to work with multiple chainstates. And brief to review. :)
Also fixes doc on ActivateBestChain.
ACKs for top commit:
MarcoFalke:
ACK 3cf36736e5
ryanofsky:
Can confirm. utACK 3cf36736e5. Removes wrapper functions and removes more ::ChainActive() and ::ChainstateActive() calls than it adds, so seems good.
Tree-SHA512: 4bf8a1dd454ca9d61c85f6736910fa7354c57acc0002e3a8e5ce494035d8280e4c20e066f03478eeff7d44195e7912c282a486526da9be53854b478b961affaa
c4b0c08f7c Update tx-size-small comment with relevant CVE disclosure (Gregory Sanders)
Pull request description:
Code first introduced under https://github.com/bitcoin/bitcoin/pull/11423 with essentially no description and no discussion.
ACKs for top commit:
MarcoFalke:
ACK c4b0c08f7c
fanquake:
ACK c4b0c08f7c
Tree-SHA512: 95d5c92998b8b1e944c477dbaee265b62612b6e815099ab31d9ff580b4dff777abaf7f326a284644709f918aa1510412d62310689b1250ef6e64de7b19ca9f71
fadfd844de test: Remove unused connect_nodes_bi (MarcoFalke)
fa3b9ee8b2 scripted-diff: test: Replace connect_nodes_bi with connect_nodes (MarcoFalke)
faaee1e39a test: Use connect_nodes when connecting nodes in the test_framework (MarcoFalke)
1111bb91f5 test: Reformat python imports to aid scripted diff (MarcoFalke)
Pull request description:
By default all test nodes are connected in a chain. However, instead of just a single connection between each pair of nodes, we end up with up to four connections for a "middle" node (two outbound, two inbound, from each side).
This is generally redundant (tx and block relay should succeed with just a single connection) and confusing. For example, test timeouts after a call to `sync_` may be racy and hard to reproduce. On top of that, the test `debug.log`s are hard to read because txs and block invs may be relayed on the same connection multiple times.
Fix this by inlining `connect_nodes_bi` in the two tests that need it, and then replace it with a single `connect_nodes` in all other tests.
Historic background:
`connect_nodes_bi` has been introduced as a (temporary?) workaround for bug #5113 and #5138, which has long been fixed in #5157 and #5662.
ACKs for top commit:
laanwj:
ACK fadfd844de
jonasschnelli:
utACK fadfd844de - more of less a cleanup PR.
promag:
Tested ACK fadfd844de, ran extended tests.
Tree-SHA512: 2d027a8fd150749c071b64438a0a78ec922178628a7dbb89fd1212b0fa34febd451798c940101155d3617c0426c2c4865174147709894f1f1bb6cfa336aa7e24
fac35b21e2 test: lint: Add DisabledOpcodeTemplates to whitelist (MarcoFalke)
Pull request description:
Fixes#16906
Top commit has no ACKs.
Tree-SHA512: 98b175bb062425fd3a8bd0d0258f4c0e0d5106980f1e037df7c2b2b2e5aa6031b11b582c026265d7b2de56049ccbadb0b7add9130d323f15886f681c6268ba0a
2dfd6834ef test: Add test for default maxfeerate in sendrawtransaction (Joonmo Yang)
261843e4be wallet/rpc: Use the default maxfeerate value as BTC/kB (Joonmo Yang)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/16382
This patch tries to treat `maxfeerate` in sendrawtransaction/testmempoolaccept RPC as a rate(BTC/kB) instead of an absolute value(BTC).
The included test case checks if the new behavior works correctly, by using the transaction with an absolute fee of ~0.02BTC, where the fee rate is ~0.2BTC/kB.
This test should be failing if the default `maxfeerate` is 0.1BTC, but pass if the default value is 0.1BTC/kB
ACKs for top commit:
laanwj:
ACK 2dfd6834ef (ACKs by Sjors and MarcoFalke above for trivially different code)
Tree-SHA512: a1795bffe8a182acef8844797955db1f60bb0c0ded97148f3572dc265234d5219271a3a7aa0b6418a43f73b2b2720ef7412ba169c99bb1cdcac52051f537d6af
