fa4bc897fc9332a5666ca2f3e78492cd67ee6128 fuzz: Fail if message type is not fuzzed (MarcoFalke)
faefed8cd5d26a485f5f6df824d7c90967c826f3 fuzz: Count message type fuzzers before main() (MarcoFalke)
Pull request description:
`process_message_*` is a nice way to quickly fuzz a single message type. However, the offered message types are outdated and all BIPs implemented in the last years are missing.
Fix that by adding them and failing when the number of message types don't add up.
ACKs for top commit:
practicalswift:
cr ACK fa4bc897fc9332a5666ca2f3e78492cd67ee6128: patch looks correct and touches only `src/test/fuzz/`
Tree-SHA512: 8c98374b50fb4ab2ff2550daeab4c6e9f486bfe847466d217d4bc97d119adc99a82b87b56f47535b1cf8f844232bc7fa1230712a9147cda514ae78851556f988
fac726b1b8331b267973138bbd2bff5304774315 doc: Fixup docs in fuzz/script_assets_test_minimizer.cpp (MarcoFalke)
fafca47adc2476f19f7926de4d55b64b0286e41c fuzz: Hide script_assets_test_minimizer (MarcoFalke)
Pull request description:
This is not an actual fuzz target. It is a hack to exploit the built-in capability of fuzz engines to measure coverage.
ACKs for top commit:
practicalswift:
cr ACK fac726b1b8331b267973138bbd2bff5304774315: patch looks correct and touches only `src/test/fuzz/`
Tree-SHA512: 0652dd8d9e95746b0906be4044467435d8204a34a30366ae9bdb75b9cb0788d429db7cedf2760fd543565d9d4f7ee206873ed10a29dd715a792a26337f65b53c
eecb7ab105a4a59d09cd55b124c5ad563846fe11 [doc] clarify -peertimeout and -timeout descriptions (gzhao408)
Pull request description:
The debug-only option `-peertimeout` is used to delay `InactivityCheck()`, whereas the `-timeout` option specifies socket timeouts (`nConnectTimeout`). The current descriptions are a bit misleading and hard to tell apart. I think it would save dev/review time to update them 🤷
ACKs for top commit:
MarcoFalke:
ACK eecb7ab105a4a59d09cd55b124c5ad563846fe11 nice doc fixup
jnewbery:
ACK eecb7ab105a4a59d09cd55b124c5ad563846fe11
Tree-SHA512: 71d2e6c31664b9f7f0b053ecf3be21c6c55472553fa7478d8526ba3be8d54979bceafca63d87b8b2488c11f409c332ac795da613ff8101546b18d9cd8bcceb50
8f0b64fb513e8c6cdd1f115856100a4ef5afe23e Better error messages for invalid addresses (Bezdrighin)
Pull request description:
This PR addresses #20809.
We add more detailed error messages in case an invalid address is provided inside the 'validateaddress' and 'getaddressinfo' RPC calls. This also covers the case when a user provides an address from a wrong network.
We also add a functional test to test the new error messages.
ACKs for top commit:
kristapsk:
ACK 8f0b64fb513e8c6cdd1f115856100a4ef5afe23e
meshcollider:
Code review ACK 8f0b64fb513e8c6cdd1f115856100a4ef5afe23e
Tree-SHA512: ca0f806ab573e96b79e98d9f8c810b81fa99c638d9b5e4d99dc18c8bd2568e6a802ec305fdfb2983574a97a19a46fd53b77645f8078fb77e9deb24ad2a22cf93
7487bc9900d28e1b5361cba882fd8783aafc7092 Fix BlockToJsonVerbose benchmark (Martin Ankerl)
Pull request description:
Currently it was not possible to run just the BlockToJsonVerbose benchmark because it did not set up everything it needed, running `bench_bitcoin -filter=BlockToJsonVerbose` caused this assert to fail:
```
bench_bitcoin: chainparams.cpp:506: const CChainParams& Params(): Assertion `globalChainParams' failed.
```
Initializing TestingSetup fixes this.
ACKs for top commit:
theStack:
Tested ACK 7487bc9900d28e1b5361cba882fd8783aafc7092 🐎
Tree-SHA512: 27b9702cb4bacc0475710f7b31f41844e83b8a0787685380749505d179aba724728604d4e4e2e3b3cb38cde88ab12f170881b5d3eb615872ee84632e85312166
… best height
## Issue being fixed or feature implemented
Platform wants to know the height of the bestchainlock when they call
submitchainlock; sooo we change the API of submitchainlock to also
return the height
## What was done?
Adjust API and tests
## How Has This Been Tested?
New tests added for this behavior
## Breaking Changes
Not really any; I **guess** that return value could be considered
breaking change; but going from nothing -> something feels unlikely to
break anything although it in theory could.
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
BACKPORT NOTICE:
define/endif moved in original backport but some changes are not backported yet
---------
fa3967efdb07f1d22372f4ee2e602ea1fad04a57 test: Replace ARRAYLEN with C++11 ranged for loop (MarcoFalke)
fafc5290538fde76c3780976f4b2c11dc9f24d19 test: Run AssetTest even if built --with-libs=no (MarcoFalke)
faf58ab139949ca35b33217d010b350c9a59c61d ci: Add --with-libs=no to one ci config (MarcoFalke)
Pull request description:
`script_assets_test` doesn't call libbitcoinconsensus, so it seems confusing to require it
ACKs for top commit:
fanquake:
ACK fa3967efdb07f1d22372f4ee2e602ea1fad04a57 - looks ok to me.
Tree-SHA512: 8744fef64c5d7dc19a0ef4ae9b3df5b3f356253bf000f12723064794c5e50df071824d436059985f1112d94c1b530b65cbeb6b8435114a91b195480620eafc59
faf2999e2515c47108dc3d376dbd1c0fce4d6103 cirrus: Use kvm to avoid spurious CI failures in the default virtualization cluster (MarcoFalke)
Pull request description:
Try to fix#20093
ACKs for top commit:
practicalswift:
ACK faf2999e2515c47108dc3d376dbd1c0fce4d6103
hebasto:
ACK faf2999e2515c47108dc3d376dbd1c0fce4d6103, the related doc:
Tree-SHA512: 156aa2ce5a5dde11570f7f90f9d51be540a5469f090033ab6f337b2c46347741469e109f9566f4b7c424339483d61192b91102021f5db38823ce4cf4428e5671
33df8d46bb4b6daca91db208d3d1629d3c1e500c ci: Build Arm64 on Travis without functional tests (Fabian Jahr)
Pull request description:
The Travis Arm64 env is a pretty big PITA for quite a while now. It simply seems to time out due to a lack of resources as far as I can tell from my research into the matter.
I have reported the issue in August and received no response: https://travis-ci.community/t/arm64-failing-without-message/9775. Others have complained about similar issues with Arm64 over the past year. The explanation for this is probably that this env is still considered to be in beta: https://docs.travis-ci.com/user/multi-cpu-architectures.
Considering the high frequency of failures (I didn't run any numbers, just based on my personal observations) and the beta status I would suggest that we shorten the runtime by removing part of the test suite until the env is more stable. I would suggest removing functional tests since it should help for sure but maybe there are other ideas.
ACKs for top commit:
MarcoFalke:
ACK 33df8d46bb4b6daca91db208d3d1629d3c1e500c
Tree-SHA512: 55b0c658b526611d16a26e9508611ddeecbfbd842ec064ada61d188a8be2f473e080c2a35746a9aae8006b11b0c2ab9dd4639b0f3004ecf821506e26c7345972
fa8e1487144eab237ffd291397355ef4801f46f8 ci: Double tsan CPU and Memory to avoid global timeout (MarcoFalke)
Pull request description:
Fix#19864
ACKs for top commit:
practicalswift:
ACK fa8e1487144eab237ffd291397355ef4801f46f8 -- patch looks correct
hebasto:
ACK fa8e1487144eab237ffd291397355ef4801f46f8, according to https://cirrus-ci.org/guide/linux/ the limits are:
Tree-SHA512: b6d522290bfe80ed7453387b811628bf42c7657aa6a84d2f5984c8bb16f9857a71eabc6b8a4d63b84227d59b41a8ed7dd85d86cae5628dc9cf6b85bd365248d7
fa0538e94db26dd84e02aac1cf174b79729dae72 ci: Set cirrus RAM to 8GB (MarcoFalke)
fa41810d0e87f9f9a2e39be238b9598be02646d0 ci: Run valgrind fuzzer on cirrus (MarcoFalke)
Pull request description:
The first commit should fix the 50min timeout in forked repos. Similar to #19424. E.g. https://travis-ci.org/github/bitcoin-core/gui/builds/718322267
The second commit should fix#19744
Top commit has no ACKs.
Tree-SHA512: c765098dfa913ca49b1d1eee99aaa83e4b9eb191b7ad5e652e3f04744fe8670dd3ef4215832b8e2b5bac0273d24f607fc275e72f566326108ba42ab57228ffd4
7bf04e358a6550ac9851f1b2d87795927fc5ff4b build: remove mostly pointless BOOST_PROCESS macro (fanquake)
Pull request description:
Performing a series of link checks for a Boost component that is
header-only doesn't make much sense, and currently means we just have
another confusing Boost macro in our tree. I'm not sure why this was
originally done this way; maybe Sjors or luke-jr can elaborate (#15382 (929cda5470f98d1ef85c05b1cad4e2fb9227e3b0))?
The macro also has the side-effect of producing confusing error
messages. i.e in #20744, the CI is currently failing with:
```bash
checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes
checking for boostlib >= 1.58.0 (105800)... yes
checking whether the Boost::Process library is available... yes
configure: error: Could not find a version of the Boost::Process library!
```
This isn't useful, given there is no such thing as a `Boost::Process` library.
This PR just removes the macro entirely, but maintains a `--with-boost-process`
(defaulting to off), flag to configure. Hopefully this will also be
removed, in favour of `--enable/disable-external-signer` if/when #16546
is merged.
ACKs for top commit:
laanwj:
ACK 7bf04e358a6550ac9851f1b2d87795927fc5ff4b
Tree-SHA512: b270a0250f32df2078f986c165b8977967d8c06df80bf2773f3442f74b395a3bfa6544af1024d9b6524d90d47a0f6304194b3aced0e2ecb88e75916da945ccb6
31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0 [util] add RunCommandParseJSON (Sjors Provoost)
c17f54ee535faaedf9033717403e1f775b5f1530 [ci] use boost::process (Sjors Provoost)
32128ba682033560d6eb2e4848a9f77a842016d2 [doc] include Doxygen comments for HAVE_BOOST_PROCESS (Sjors Provoost)
3c84d85f7d218fa27e9343c5cd1a55e519218980 [build] msvc: add boost::process (Sjors Provoost)
c47e4bbf0b44f2de1278f9538124ec98ee0815bb [build] make boost-process opt-in (Sjors Provoost)
929cda5470f98d1ef85c05b1cad4e2fb9227e3b0 configure: add ax_boost_process (Sjors Provoost)
8314c23d7b39fc36dde8b40b03b6efbe96f85698 [depends] boost: patch unused variable in boost_process (Sjors Provoost)
Pull request description:
Prerequisite for external signer support in #16546. Big picture overview in [this gist](https://gist.github.com/Sjors/29d06728c685e6182828c1ce9b74483d).
This adds a new dependency [boost process](https://github.com/boostorg/process/tree/boost-1.64.0). This is part of Boost since 1.64 which is part of `depends`. Because the minimum Boost version is 1.47, this functionality is skipped for older versions of Boost.
Use `./configure --with-boost-process` to opt in, which checks for the presence of Boost::Process.
We add `UniValue runCommandParseJSON(const std::string& strCommand)` to `system.{h,cpp}` which calls an arbitrary command and processes the JSON returned by it. This is currently only called by the test suite.
~For testing purposes this adds a new regtest-only RPC method `runcommand`, as well as `test/mocks/command.py` used by functional tests.~ (this is no longer the case)
TODO:
- [ ] review boost process in #15440
ACKs for top commit:
achow101:
ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0
hebasto:
re-ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0, only rebased (verified with `git range-diff`) and removed an unintentional tab character since the [previous](https://github.com/bitcoin/bitcoin/pull/15382#pullrequestreview-458371035) review.
meshcollider:
Very light utACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0, although I am not very confident with build stuff.
promag:
Code review ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0, don't mind the nit.
ryanofsky:
Code review ACK 31cf68a3ad1f0a5537c8419e2912b55fbfb88fa0. I left some comments below that could be ignored or followed up later. The current change is clean and comprehensive.
Tree-SHA512: c506e747014b263606e1f538ed4624a8ad7bcf4e025cb700c12cc5739964e254dc04a2bbb848996b170e2ccec3fbfa4fe9e2b3976b191222cfb82fc3e6ab182d
a76dafa51dd16e3f1ed665f6f7b6b2b6a708b9b4 ci: Add tsan suppression for race in BerkeleyBatch (Hennadii Stepanov)
Pull request description:
A temporary workaround for #19448.
Top commit has no ACKs.
Tree-SHA512: 47b83ff373e710bc9ba8c3661f9850a14417436028c42eb7765d21337ef25faaac4cf8cf93be844ae592d40264934d7d2f6b7ba0ab6c7209fc0da8fc13067769
fa8e6df282af0d396d75b03721f1b59a520ced19 ci: Run tsan ci config on cirrus (MarcoFalke)
Pull request description:
Fixes bitcoin-core/gui#12
Copied description from #19321:
Currently it is not possible to use travis in forked repositories due to the 50 minute limit on builds. A fresh build (uncached) of the thread sanitizer config takes more than 50 minutes.
One approach to fix this could be to throw away tests until the run time is less than 50 minutes. However, the risk of being blind of failures in the thrown away tests is not worth the gain. Also, to detect them, one has to run the tsan configuration nightly and failures could only be detected post-merge.
Another approach would be to ask travis support to raise the limit for a forked repository. This is a tedious and manual one-by-one process, so I'd rather not.
Finally, a different ci provider can be used, since the config files are designed to be platform-agnostic. This is what I picked.
I kept all settings identical to the travis machine for now. Both providers run in the google cloud, so this should be a "move-only".
ACKs for top commit:
fanquake:
ACK fa8e6df282af0d396d75b03721f1b59a520ced19 - my understanding is that test coverage remains the same. Just swapping providers to work-around the Travis time-limit in other repos.
Tree-SHA512: 26fed248a4f743107160d3b9e5df57fa0be280fd065ae6fece83d254f59d58ccf3e11a245519d158da109c47b053f62ee8756215008541973c65dc28c4efb748
a92e48b02df545e620a7b1de74f647f46413d3fb test: move TEST_RUNNER_EXTRA into native tsan setup (fanquake)
Pull request description:
`feature_block.py` is being run in the tsan job, i.e [here](https://travis-ci.org/github/bitcoin/bitcoin/jobs/703122309), even though it should be excluded. My hasty assumption is that this will fix it. In any case, all other instances of `TEST_RUNNER_EXTRA` seem to have moved out of `.travis.yml` and into the different CI configurations.
ACKs for top commit:
MarcoFalke:
ACK a92e48b02df545e620a7b1de74f647f46413d3fb
practicalswift:
ACK a92e48b02df545e620a7b1de74f647f46413d3fb -- patch looks correct
hebasto:
ACK a92e48b02df545e620a7b1de74f647f46413d3fb, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 86057bef2cc87c6acdbbf94f8cd7a5147510448c3e67aacde8daf247e3ccf649cfc5afbbd10693e084f426042d98150616c0e49bfa5f32b949dff9cebd2fd95d
fa2eb3d5d6819e42bfcec8a9f02b99438fe718b9 ci: Run asan ci config on cirrus (MarcoFalke)
fa93527738a62ebc13305adcb0fd2b5128073bbc cirrus: Clear dummy task (MarcoFalke)
Pull request description:
Currently it is not possible to use travis in forked repositories due to the 50 minute limit on builds. A fresh build (uncached) of the address sanitizer config takes more than 50 minutes.
One approach to fix this could be to throw away tests until the run time is less than 50 minutes. However, the risk of being blind of failures in the thrown away tests is not worth the gain. Also, to detect them, one has to run the asan configuration nightly and failures could only be detected post-merge.
Another approach would be to ask travis support to raise the limit for a forked repository. This is a tedious and manual one-by-one process, so I'd rather not.
Finally, a different ci provider can be used, since the config files are designed to be platform-agnostic. This is what I picked.
I kept all settings identical to the travis machine for now. Both providers run in the google cloud, so this should be a "move-only".
ACKs for top commit:
hebasto:
ACK fa2eb3d5d6819e42bfcec8a9f02b99438fe718b9
Tree-SHA512: 159d7dc6f5b24583e941282cdd40465b15db787f0a658a3e81a7b1a22abdb4cb573709b9b5c4465523e0ba0060b17a68fbdbda7a9ecdeb649f31535d377bbe75
## Issue being fixed or feature implemented
Dashmate wanted a way to know if it is safe to restart the masternode.
This new RPC indicates the number of active DKG sessions, and the number
of blocks until next potential DKG.
## What was done?
Examples of responses:
`{'active_dkgs': 0, 'next_dkg': 22}`
## How Has This Been Tested?
`feature_llmq_rotation.py` was updated
## Breaking Changes
no
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
RPC `getassetunlockstatuses` is now accepting an extra optional
parameter `height`.
When a valid `height` is passed, then the RPC returns the status of
AssetUnlock indexes up to this specific block. (Requested by Platform
team)
## What was done?
Note that in order to avoid cases that can lead to deterministic result,
when `height` is passed, then the only `chainlocked` and `unknown`
outcomes are possible.
## How Has This Been Tested?
`feature_asset_locks.py` was updated.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: Konstantin Akimov <knstqq@gmail.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
Trivial bug: `keypoolrefill` internally updates the status of the
progress bar each second.
```
m_storage.UpdateProgress(strMsg, static_cast<int>(dProgress));
```
However it can happen that one second is not enough time to make
significant progress and
`static_cast<int>(dProgress) = 0`.
Calling the function with `0` as progress opens a new progress bar and
this led to problems like #5730
## What was done?
trivially make sure to update the progress only if the parameter is >0
## How Has This Been Tested?
rpc does not spam anymore
## Checklist:
_Go over all the following points, and put an `x` in all the boxes that
apply._
- [X] I have performed a self-review of my own code
- [X] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
519cae8fd6e44aef3470415d7c5e12acb0acd9f4 gui: Delay interfaces::Node initialization (Russell Yanofsky)
102abff9eb6c267af64f2a3560712147d1896e13 gui: Replace interface::Node references with pointers (Russell Yanofsky)
91aced7c7e6e75c1f5896b7e3843015177f32748 gui: Remove unused interfaces::Node references (Russell Yanofsky)
e1336316250ab5cb0ed654b1e593378a6e0769ce gui: Partially revert #10244 gArgs and Params changes (Russell Yanofsky)
Pull request description:
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10).
---
This is a partial revert of https://github.com/bitcoin/bitcoin/pull/10244. It changes gui code to go back to using gArgs and Params() functions directly instead of using interfaces::Node to handle arguments.
These changes were originally pushed as part of https://github.com/bitcoin/bitcoin/pull/19461. Motivation is to support a new GUI process connecting to an already running node process. Details are explained in commit messages, but in addition to spawning a new bitcoin-node process, we want bitcoin-gui to connect to an existing bitcoin-node process. So for that reason it should be able to parse its own parameters, rather than rely on the node.
ACKs for top commit:
MarcoFalke:
re-ACK 519cae8fd6, only change is rebase and addressed nits of my previous review last week 🌄
Tree-SHA512: 9c339dd82ba78bcc7b887b84d872f35ccc7dfa3d271691e6eafe8a2048cbbe3bdde1e810ce33d0714d75d048c9de3470e9e9b6f8306a6047d1cb3548f6858dc8
241434200ec2067673d8522fee4f1228abfd8247 refactor: qt: Use vQueueNotifications.clear() (João Barbosa)
989e579d07bb5031639060b717f7a0be15d10e29 qt: Make transaction notification queue wallet specific (João Barbosa)
7b3b2303f44031c3545651858f697a495c3ea37a move-only: Define TransactionNotification before TransactionTablePriv (João Barbosa)
Pull request description:
Currently `vQueueNotifications` holds transactions of any wallet, but the queue is dispatched on a given wallet and it assumes notifications are of that wallet.
This means that some transactions can be missed if multiple wallets are loaded.
Fix this by having a queue for each wallet.
ACKs for top commit:
jonasschnelli:
utACK 241434200ec2067673d8522fee4f1228abfd8247
hebasto:
ACK 241434200ec2067673d8522fee4f1228abfd8247, I have reviewed the code and it looks OK, I agree it can be merged.
ryanofsky:
Code review ACK 241434200ec2067673d8522fee4f1228abfd8247. Only change is dropping one commit
Tree-SHA512: 61beac5a16ed659e3a25ad145dbceafcef963aaf8f9838355298949ec2324e2bd760f59353cd251d30cf0334d8dc1642a1f3821d8a9eec092533b581f6ce86db
1afcd41a906e6417925e80578c0d850d269dc008 [net] Remove CombinerAll (John Newbery)
Pull request description:
This was introduced in 9519a9a4 for use with boost signals. Boost signals
have not been used in net since 8ad663c1, so this code is unused.
ACKs for top commit:
MarcoFalke:
review ACK 1afcd41a906e6417925e80578c0d850d269dc008
laanwj:
code review ACK 1afcd41a906e6417925e80578c0d850d269dc008
Tree-SHA512: a4313142afb88bf12f15abc4e717b3b0d0b40d2d5db2638494af3181e1cd680d7b036087050fc0e0dfe606228849a2e20ae85135908a9ebe8ff2130f163920e1
82dee87933ed0714976ff4eb9657acfc13c6de84 test: test decodepsbt fee calculation (count input value only once per UTXO) (Sebastian Falbesoner)
Pull request description:
Fixes#19523, adding a simple test to `rpc_psbt.py` that checks that the decodepsbt fee matches the one given by the wallet (`walletcreatefundedpsbt`). This is in particular important for PSBTs with segwit inputs that have both a witness- and a non-witness-UTXO type set.
Example test run after reverting commit 75122780e2c46505d977e24c5612dfa9442ab754 ("Increment input value sum only once per UTXO in decodepsbt"):
```
$ test/functional/rpc_psbt.py
2020-07-26T11:31:44.862000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__sutcd4y
20.00007580
2020-07-26T11:31:47.073000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/honeybadger/buidl/bitcoin_thestack/test/functional/test_framework/test_framework.py", line 118, in main
self.run_test()
File "test/functional/rpc_psbt.py", line 166, in run_test
assert_equal(decoded['fee'], created_psbt['fee'])
File "/home/honeybadger/buidl/bitcoin_thestack/test/functional/test_framework/util.py", line 49, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(20.00007580 == 0.00007580)
2020-07-26T11:31:47.125000Z TestFramework (INFO): Stopping nodes
......
```
ACKs for top commit:
achow101:
ACK 82dee87933ed0714976ff4eb9657acfc13c6de84
Tree-SHA512: 296b8a701f851d482ef6200c6cbf0cf0257a79a828ac6dbc39b05d8c2d839c6fdb9d3f5a084015295cfa3eac7c11faa2f2d52e619c11627b04c75150eead8330
88df300f20da02060694cfe643e1c882efaa306c qt: Do not translate file extensions (Hennadii Stepanov)
Pull request description:
File extensions are untranslatable by their nature.
ACKs for top commit:
laanwj:
Concept and code review ACK 88df300f20da02060694cfe643e1c882efaa306c
Talkless:
tACK 88df300f20da02060694cfe643e1c882efaa306c, tested on Debian Sid with Qt 5.15.2. Tested all filters except for .psbt.
jarolrod:
re-ACK 88df300f20da02060694cfe643e1c882efaa306c
Tree-SHA512: 104d383543edcee8fb825f98d3b6669a7aaae2c74b6602f9bc407bf1c88be121ec535f2f9be87afa6ca775dc79865165f620553f6f6ab1d31a3f9ea93f7f9593
ac7ccd67d7f2b09e36dd57405f899e4698dd3d78 scripted-diff: Remove unused "What's This" button in dialogs on Windows (Hennadii Stepanov)
b6951483ecdd4409a0e1d492c93bcd4d823f039d qt: Add flags to prevent a "What's This" button on Windows OS (Hennadii Stepanov)
Pull request description:
Fix#74.
From [Qt docs](https://doc.qt.io/qt-5/qdialog.html#QDialog):
> The widget flags _f_ are passed on to the `QWidget` constructor. If, for example, you don't want a **What's This** button in the title bar of the dialog, pass `Qt::WindowTitleHint | Qt::WindowSystemMenuHint` in _f_.
Screenshot on Windows 10 (2004):
- master (3ba25e3bdde3464eed5d2743d68546e48b005544)
- this PR (e322fe7e19ac504272d14b9b4f9b28b13df888ed)
ACKs for top commit:
Bosch-0:
tACK ac7ccd67d7f2b09e36dd57405f899e4698dd3d78 Tested on Windows 10.0.18363 Build 18363.
promag:
Code review ACK ac7ccd67d7f2b09e36dd57405f899e4698dd3d78 but with some suggestions.
jonasschnelli:
utACK ac7ccd67d7f2b09e36dd57405f899e4698dd3d78
Tree-SHA512: f6750a17b7203106cb4db5870becba1cef6a505d4edcc710ba131338bd3aae051510627e62c9bcb8345a7f497c614709e11aeb8f6ae3ea85967bbce2a8c69e64
BACKPORT NOTICE
fixup psbt. all missing changes belongs to src/wallet/scriptpubkeyman.h/cpp ----- they are related to descriptor wallet!
-------------------
931dd4760855e036c176a23ec2de367c460e4243 Make lint-spelling.py happy (Glenn Willen)
11a0ffb29d1b4dcc55c8826873f340ab4196af21 [gui] Load PSBT from clipboard (Glenn Willen)
a6cb0b0c29d327d01aebb98b0504f317eb19c3dc [gui] PSBT Operations Dialog (sign & broadcast) (Glenn Willen)
5dd0c03ffa3aeaa69d8a3a716f902f450d5eaaec FillPSBT: report number of inputs signed (or would sign) (Glenn Willen)
9e7b23b73387600d175aff8bd5e6624dd51f86e7 Improve TransactionErrorString messages. (Glenn Willen)
Pull request description:
Add a "PSBT Operations" dialog, reached from the "Load PSBT..." menu item, giving options to sign or broadcast the loaded PSBT as appropriate, as well as copying the result to the clipboard or saving it to a file.
This is based on Sjors' #17509, and depends on that PR going in first. (It effectively replaces the small "load PSBT" dialog from that PR with a more feature-rich one.)
Some notes:
* The way I display status information is maybe unusual (a status bar, rather than messageboxes.) I think it's helpful to have the information in it be persistent rather than transitory. But if people dislike it, I would probably move the "current state of the transaction" info to the top line of the main label, and the "what action just happened, and did it succeed" info into a messagebox.
* I don't really know much about the translation/localization stuff. I put tr() in all the places it seemed like it ought to go. I did not attempt to translate the result of TransactionErrorString (which is shared by GUI and non-GUI code); I don't know if that's correct, but it matches the "error messages in logs should be googleable in English" heuristic. I don't know whether there are things I should be doing to reduce translator effort (like minimizing the total number of distinct message strings I use, or something.)
* I don't really know how (if?) automated testing is applied to GUI code. I can make a list of PSBTs exercising all the codepaths for manual testing, if that's the right approach. Input appreciated.
ACKs for top commit:
instagibbs:
tested ACK 931dd47608
Sjors:
re-tACK 931dd4760855e036c176a23ec2de367c460e4243
jb55:
ACK 931dd4760855e036c176a23ec2de367c460e4243
achow101:
ACK 931dd4760855e036c176a23ec2de367c460e4243
Tree-SHA512: ade52471a2242f839a8bd6a1fd231443cc4b43bb9c1de3fb5ace7c5eb59eca99b1f2e9f17dfdb4b08d84d91f5fd65677db1433dd03eef51c7774963ef4e2e74f
5da96210fc2fda9fbd79531f42f91262fd7a9257 doc: release note for getpeerinfo last_block/last_transaction (Jon Atack)
cfef5a2c98b9563392a4a258fedb8bdc869c9749 test: rpc_net.py logging and test naming improvements (Jon Atack)
21c57bacda766a4f56ee75a2872f5d0f94e3901e test: getpeerinfo last_block and last_transaction tests (Jon Atack)
8a560a7d57cbd9f473d6a3782893a0e2243c55bd rpc: expose nLastBlockTime/TXTime as getpeerinfo last_block/transaction (Jon Atack)
02fbe3ae0bd91cbab2828cb7aa46f6493c82f026 net: add nLastBlockTime/TXTime to CNodeStats, CNode::copyStats (Jon Atack)
Pull request description:
This PR adds inbound peer eviction criteria `nLastBlockTime` and `nLastTXTime` to `CNodeStats` and `CNode::copyStats`, which then allows exposing them in the next commit as `last_transaction` and `last_block` Unix Epoch Time fields in RPC `getpeerinfo`.
This may be useful for writing missing eviction tests. I'd also like to add `lasttx` and `lastblk` columns to the `-netinfo` dashboard as described in https://github.com/bitcoin/bitcoin/pull/19643#issuecomment-671093420.
Relevant discussion at the p2p irc meeting http://www.erisian.com.au/bitcoin-core-dev/log-2020-08-11.html#l-549:
```text
<jonatack> i was specifically trying to observe and figure out how to test https://github.com/bitcoin/bitcoin/issues/19500
<jonatack> which made me realise that i didn't know what was going on with my peer conns in enough detail
<jonatack> i'm running bitcoin locally with nLastBlockTime and nLastTXTime added to getpeerinfo for my peer connections dashboard
<jonatack> sipa: is there a good reason why that (eviction criteria) data is not exposed through getpeerinfo currently?
<sipa> jonatack: nope; i suspect just nobody ever added it
<jonatack> sipa: thanks. will propose.
```
The last commit is optional, but I think it would be good to have logging in `rpc_net.py`.
ACKs for top commit:
jnewbery:
Code review ACK 5da96210fc2fda9fbd79531f42f91262fd7a9257
theStack:
Code Review ACK 5da96210fc2fda9fbd79531f42f91262fd7a9257
darosior:
ACK 5da96210fc2fda9fbd79531f42f91262fd7a9257
Tree-SHA512: 2db164bc979c014837a676e890869a128beb7cf40114853239e7280f57e768bcb43bff6c1ea76a61556212135281863b5290b50ff9d24fce16c5b89b55d4cd70
## Issue being fixed or feature implemented
`develop` can't sync from genesis on mainnet,
b8a086d5e7
broke it.
#5790 follow-up
## What was done?
Revive the old logic but using hardcoded block heights instead of
scanning via quorum manager.
## How Has This Been Tested?
Synced on mainnet/testnet, CI is happy
https://gitlab.com/UdjinM6/dash/-/pipelines/1148980046.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
f32c408f3a0b7e597977df2bc2cdc4ae298586e5 Make sure unconfirmed parents are requestable (Pieter Wuille)
c4626bcd211af08c85b6567ef07eeae333edba47 Drop setInventoryTxToSend based filtering (Pieter Wuille)
43f02ccbff9b137d59458da7a8afdb0bf80e127f Only respond to requests for recently announced transactions (Pieter Wuille)
b24a17f03982c9cd8fd6ec665b16e022374c96f0 Introduce constant for mempool-based relay separate from mapRelay caching (Pieter Wuille)
a9bc5638031a29abaa40284273a3507b345c31e9 Swap relay pool and mempool lookup (Pieter Wuille)
Pull request description:
This implements the follow-up suggested here: https://github.com/bitcoin/bitcoin/pull/18861#issuecomment-627630111 . Instead of checking `setInventoryTxToSend`, maintain an explicit bloom filter with the 3500 most recently announced invs, and permit fetching any of these as long as they're in the relay pool or the mempool. In addition, permit relay from the mempool after just 2 minutes instead of 15.
This:
* Fixes the brief opportunity an attacker has to request unannounced invs just after the connection is established (pointed out by naumenkogs, see https://github.com/bitcoin/bitcoin/pull/18861#issuecomment-627627010).
* Guarantees that locally resubmitted invs after `filterInventoryKnown` rolls over can still be requested (pointed out by luke-jr, see https://github.com/bitcoin/bitcoin/pull/18861#discussion_r419695831).
It adds 37 KiB of filter per peer.
This is also a step towards dropping the relay pool entirely and always relaying from the mempool directly (see #17303), but that is still blocked by dealing properly with NOTFOUNDs (see #18238).
ACKs for top commit:
jnewbery:
reACK f32c408f3
jonatack:
re-ACK f32c408 per `git range-diff f7c19e8 2da7ee3 f32c408` and redid the following: code review, thought about motivation, DoS and privacy aspects, debug build to check for warnings after updating Clang from 6 to 11 since last review.
ajtowns:
re-ACK f32c408f3a0b7e597977df2bc2cdc4ae298586e5
Tree-SHA512: aa05b9fd01bad59581c4ec91836a52d7415dc933fa49d4c4adced79aa25aaad51e11166357e8c8b29fbf6021a7401b98c21b850b5d8e8ad773fdb5d6608e1e85
41dca087b73a3627107603694f5a982ea2a53189 [trivial] Extract connection type doc into file where it is used. (Amiti Uttarwar)
3069b56a456d98fca7c4a4ccd329581bd1f0b853 [doc] Improve help for getpeerinfo connection_type field. (Amiti Uttarwar)
Pull request description:
two commits addressing small followups from #19725
* first commit adds a clarification in the release notes that this field shouldn't be expected to be stable (suggested by sdaftuar in https://github.com/bitcoin/bitcoin/pull/19725#issuecomment-697421878)
* second commit moves the `CONNECTION_TYPE_DOC` object out of the header file to reduce the size of the binary (suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/19725#discussion_r495467895, he tested and found a decrease of 10kB)
ACKs for top commit:
achow101:
ACK 41dca087b73a3627107603694f5a982ea2a53189
laanwj:
Code review ACK 41dca087b73a3627107603694f5a982ea2a53189
Tree-SHA512: a555df978b4341fbe05deeb40a8a655f0d3c5c1c0adcc1737fd2cf61b204a5a24a301ca0c2b5a3616554d4abf8c57074d22dbda5a50d8450bc22c57679424985
a490d074b3491427afbd677f5fa635b910f8bb34 doc: Add anchors.dat to files.md (Hennadii Stepanov)
0a85e5a7bc8dc6587963e2e37ac1b087a1fc97fe p2p: Try to connect to anchors once (Hennadii Stepanov)
5543c7ab285e90256cbbf9858249e028c9611cda p2p: Fix off-by-one error in fetching address loop (Hennadii Stepanov)
4170b46544231e7cf1d64ac3baa314083be37502 p2p: Integrate DumpAnchors() and ReadAnchors() into CConnman (Hennadii Stepanov)
bad16aff490dcf87722fbfe202a869fb24c734e1 p2p: Add CConnman::GetCurrentBlockRelayOnlyConns() (Hennadii Stepanov)
c29272a157d09a8125788c1b860e89b63b4cb36c p2p: Add ReadAnchors() (Hennadii Stepanov)
567008d2a0c95bd972f4031f31647c493d1bc2e8 p2p: Add DumpAnchors() (Hennadii Stepanov)
Pull request description:
This is an implementation of #17326:
- all (currently 2) outbound block-relay-only connections (#15759) are dumped to `anchors.dat` file
- on restart a node tries to connect to the addresses from `anchors.dat`
This PR prevents a type of eclipse attack when an attacker exploits a victim node restart to force it to connect to new, probably adversarial, peers.
ACKs for top commit:
jnewbery:
code review ACK a490d074b3
laanwj:
Code review ACK a490d074b3491427afbd677f5fa635b910f8bb34
Tree-SHA512: 0f5098a3882f2814be1aa21de308cd09e6654f4e7054b79f3cfeaf26bc02b814ca271497ed00018d199ee596a8cb9b126acee8b666a29e225b08eb2a49b02ddd
faad92fe1c3cca9795226bd167130976930ddab8 test: Remove unused nVersion=1 in p2p tests (MarcoFalke)
Pull request description:
After commit ddefb5c0b759950942ac03f28c43b548af7b4033 nVersion is no
longer used in p2p logic when sending messages. Only when receiving
messages, but in this test no messages are received.
ACKs for top commit:
laanwj:
Code review ACK faad92fe1c3cca9795226bd167130976930ddab8
fanquake:
ACK faad92fe1c3cca9795226bd167130976930ddab8
Tree-SHA512: 9a7029187aaa5a7929a4a2199646131ff1ea72df6a855ce7022dd3bb2647dd525356dbc5e460c77007eebcdeab400a689db8cb77e8239af3b539c117a4e0d16e
b6834e312a6a7bb395ec7266bc9469384639df96 Avoid 'timing mishap' warnings when mocking (Pieter Wuille)
ec3916f40a3fc644ecbbaaddef6258937c7fcfbc Use mockable time everywhere in net_processing (Pieter Wuille)
Pull request description:
The fact that net_processing uses a mix of mockable tand non-mockable time functions made it hard to write functional tests for #19988.
I'm opening this as a separate PR as I believe it's independently useful. In some ways this doesn't go quite as far as it could, as there are now several data structures that could be converted to `std::chrono` types as well now. I haven't done that here, but I'm happy to reconsider that.
ACKs for top commit:
MarcoFalke:
ACK b6834e312a 🌶
jnewbery:
utACK b6834e312a6a7bb395ec7266bc9469384639df96
naumenkogs:
utACK b6834e3
Tree-SHA512: 6528a167c57926ca12894e0c476826411baf5de2f7b01c2125b97e5f710e620f427bbb13f72bdfc3de59072e56a9c1447bce832f41c725e00e81fea019518f0e
0fcaf731997c4989b869e42d8990f742637799c2 test: use explicit p2p objects where available (Oliver Gugger)
Pull request description:
This is a follow-up patch to #19804 as suggested by MarcoFalke (https://github.com/bitcoin/bitcoin/pull/19804#discussion_r494950062).
To make the intent of the tests easier to understand, we reference the
p2p connection objects by their explicit names instead of the p2ps array.
ACKs for top commit:
theStack:
ACK 0fcaf731997c4989b869e42d8990f742637799c2
Tree-SHA512: 37db22185077beeadfa7245bb768b87d6b7a2cfb906c3c859ab92ec3d657122db7301777f0838e13dfc748f37303850144fb7553e6cb6c66903e304d6e10e659
10d61505fe77880d6989115defa5e08417f3de2d [test] remove confusing p2p property (gzhao408)
549d30faf04612d9589c81edf9770c99e3221885 scripted-diff: replace p2p with p2ps[0] in p2p_invalid_tx (gzhao408)
7a0de46aeafb351cffa3410e1aae9809fd4698ad [doc] sample code for test framework p2p objects (gzhao408)
784f757994c1306bb6584b14c0c78617d6248432 [refactor] clarify tests by referencing p2p objects directly (gzhao408)
Pull request description:
The `TestNode` has a `p2p` property which is an alias for `p2ps[0]`.
I think this should be removed because it can be confusing and misleading (to both the test writer and reviewer), especially if a TestNode has multiple p2ps connected (which is the case for many tests).
Another example is when a test has multiple subtests that connect 1 p2p and use the `p2p` property to reference it. If the subtests don't completely clean up after themselves, the subtests may affect one another.
The best way to refer to a connected p2p is use the object returned by `add_p2p_connection` like this:
```py
p2p_conn = node.add_p2p_connection(P2PInterface())
```
A good example is [p2p_invalid_locator.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_invalid_locator.py), which cleans up after itself (waits in both `wait_for_disconnect` and in `disconnect_p2ps`) but wouldn't need so much complexity if it just referenced the connections directly.
If there is only one connected, it's not really that tedious to just use `node.p2ps[0]` instead of `node.p2p` (and it can always be aliased inside the test itself).
ACKs for top commit:
robot-dreams:
utACK 10d61505fe77880d6989115defa5e08417f3de2d
jnewbery:
utACK 10d61505fe77880d6989115defa5e08417f3de2d
guggero:
Concept ACK 10d61505.
Tree-SHA512: 5965548929794ec660dae03467640cb2156d7d826cefd26d3a126472cbc2494b855c1d26bbb7b412281fbdc92b9798b9765a85c27bc1a97f7798f27f64db6f13
ddefb5c0b759950942ac03f28c43b548af7b4033 p2p: Use the greatest common version in peer logic (Hennadii Stepanov)
e084d45562b94827b3a7873895882fcaae9f4d48 p2p: Remove SetCommonVersion() from VERACK handler (Hennadii Stepanov)
8d2026796a6f7add0c2cda9806e759817d1eae6f refactor: Rename local variable nSendVersion (Hennadii Stepanov)
e9a6d8b13b0558b17cdafbd32fd2663b4138ff11 p2p: Unify Send and Receive protocol versions (Hennadii Stepanov)
Pull request description:
On master (6fef85bfa3cd7f76e83b8b57f9e4acd63eb664ec) `CNode` has two members to keep protocol version:
- `nRecvVersion` for received messages
- `nSendVersion` for messages to send
After exchanging with `VERSION` and `VERACK` messages via protocol version `INIT_PROTO_VERSION`, both nodes set `nRecvVersion` _and_ `nSendVersion` to _the same_ value which is the greatest common protocol version.
This PR:
- replaces two `CNode` members, `nRecvVersion` `nSendVersion`, with `m_greatest_common_version`
- removes duplicated getter and setter
There is no change in behavior on the P2P network.
ACKs for top commit:
jnewbery:
ACK ddefb5c0b759950942ac03f28c43b548af7b4033
naumenkogs:
ACK ddefb5c0b759950942ac03f28c43b548af7b4033
fjahr:
Code review ACK ddefb5c0b759950942ac03f28c43b548af7b4033
amitiuttarwar:
code review but untested ACK ddefb5c0b7
benthecarman:
utACK `ddefb5c`
Tree-SHA512: 5305538dbaa5426b923b0afd20bdef4f248d310855d1d78427210c00716c67b7cb691515c421716b6157913e453076e293b10ff5fd2cd26a8e5375d42da7809d
a8a64acaf32ac21feeb885671772282b531ef9a2 [BroadcastTransaction] Remove unsafe move operator (Amiti Uttarwar)
125c0381266e0e05a408f8e1818501ab73d29110 [p2p] Remove dead code (Amiti Uttarwar)
fc66d0a65cdc52a3b259effe0c29b5eafb1b5ff5 [p2p] Check for nullptr before dereferencing pointer (Adam Jonas)
cb79b9dbf4cd06e17c8c65b36bf15c3ea2641de4 [mempool] Revert unbroadcast set to tracking just txid (Amiti Uttarwar)
Pull request description:
Addresses some outstanding review comments from #18044
- reverts unbroadcast txids to a set instead of a map (simpler, communicates intent better, takes less space, no efficiency advantages of map)
- adds safety around two touchpoints (check for nullptr before dereferencing pointer, remove an inaccurate std::move operator)
- removes some dead code
Links to comments on wtxid PR: [1](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460495254) [2](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r460496023) [3](https://github.com/bitcoin/bitcoin/pull/18044#discussion_r463532611)
thanks to jnewbery & adamjonas for flagging these ! !
ACKs for top commit:
sdaftuar:
utACK a8a64acaf32ac21feeb885671772282b531ef9a2
naumenkogs:
utACK a8a64acaf32ac21feeb885671772282b531ef9a2
jnewbery:
utACK a8a64acaf32ac21feeb885671772282b531ef9a2
Tree-SHA512: 7be669cb30cc17fb9e06b50e636ef7887c6a27354697987e4e4d38dba4b8f50e175647587430cd9bc3295bec01ce8b1e6639a50a4249d8fff9b1ca1b9ead3277
fb56d37612dea6666e7da73d671311a697570dae p2p: ensure inv is GenMsgTx before ToGenTxid in inv processing (John Newbery)
aa3621385ee66c9dde5c632c0a79fba3a6ea2d62 test: use CInv::MSG_WITNESS_TX flag in p2p_segwit (Jon Atack)
24ee4f01eadb870435712950a1364cf0def06e9f p2p: make gtxid(.hash) and fAlreadyHave localvars const (Jon Atack)
b1c855453bf2634e7fd9b53c4a76a8536fc9865d p2p: use CInv block message helpers in net_processing.cpp (Jon Atack)
acd66421671e42a58e8e067868e1ab86268e3231 [net processing] Change AlreadyHaveTx() to take a GenTxid (John Newbery)
5fdfb80b861e0de3fcf8a57163b3f52af4b2df3b [net processing] Change AlreadyHaveBlock() to take block_hash argument (John Newbery)
430e183b89d00b4148f0b77a6fcacca2cd948202 [net processing] Remove mempool argument from AlreadyHaveBlock() (John Newbery)
42ca5618cae0fd9ef97d2006b17d896bc58cc17c [net processing] Split AlreadyHave() into separate block and tx functions (John Newbery)
39f1dc944554218911b0945fff7e6d06f3dab284 p2p: remove nFetchFlags from NetMsgType TX and INV processing (Jon Atack)
471714e1f024fb3b4892a7a8b34a76b83a13fa19 p2p: add CInv block message helper methods (Jon Atack)
Pull request description:
Building on #19590 and the recent `wtxid` and `GenTxid` changes, this is a refactoring and cleanup PR to simplify and improve some of the net processing code.
Some of the diffs are best reviewed with `-w` to ignore spacing.
Co-authored by John Newbery.
ACKs for top commit:
laanwj:
Code review ACK fb56d37612dea6666e7da73d671311a697570dae
jnewbery:
utACK fb56d37612dea6666e7da73d671311a697570dae
vasild:
ACK fb56d3761
Tree-SHA512: ba39b58e6aaf850880a842fe5f6295e9f1870906ef690206acfc17140aae2ac854981e1066dbcd4238062478762fbd040ef772fdc2c50eea6869997c583e6a6d
