fa5288cf5c50d014f046b7741f6da69529037933 contrib: Fixup valgrind suppressions file (MarcoFalke)
Pull request description:
I am observing this one on bionic with system boost::fs:
```
{
<insert_a_suppression_name_here>
Memcheck:Cond
fun:__wcsnlen_avx2
fun:wcsnrtombs
fun:_ZNKSt7codecvtIwc11__mbstate_tE6do_outERS0_PKwS4_RS4_PcS6_RS6_
fun:_ZN5boost10filesystem11path_traits7convertEPKwS3_RNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEERKSt7codecvtIwc11__mbstate_tE
fun:_ZN5boost10filesystem6detail11unique_pathERKNS0_4pathEPNS_6system10error_codeE
...
ACKs for top commit:
practicalswift:
ACK fa5288cf5c50d014f046b7741f6da69529037933 -- patch looks correct
Tree-SHA512: 067e10e932a7f5b13e516134e0cfd3030265c1b582cdfde1cea97042e31399aa40c4590710a39429854c68ad703a0ae9f0b06e9af4cdd81e3cacb042939a84b6
3f686d1a286f88af7baa17c0c9dc439559a66e81 ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors (practicalswift)
Pull request description:
Re-introduce the Travis valgrind fuzzing job which was removed by PR #18899. The removal seems to have been made by accident since the removed job does not appear to be the source of the problem the PR set out to fix.
---
Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`.
This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (#18162) and similar cases.
This fuzzing job was introduced in #18166.
Top commit has no ACKs.
Tree-SHA512: 6e2681eb0ade6af465c5ea91ac163a337465d2130ec9880ba57a36d9af7c25682734586a32977dc25972d4f78483f339d680ea48c0ae13cf1dfa52b617aae401
faceeae49ae85fb644996fedb970ad9d0e297ca5 ci: Disable valgrind functionl tests on forked repos to avoid timeouts (MarcoFalke)
Pull request description:
Allows people to fork our repo and run the tests again
Also print more cache stats
ACKs for top commit:
hebasto:
ACK faceeae49ae85fb644996fedb970ad9d0e297ca5, tested on my own repo: https://travis-ci.org/github/hebasto/bitcoin/jobs/676257500
Tree-SHA512: 50e44edf94fcb997438eeaf7308b2b58a0854141ecb1fbb0ba7bf5ed662f19b60899f966f579cca90ad5e789234d0e90122d8c2c854da70339058adc3a475fa6
6136a96cdf631f824fb89cf2720422787439acc9 ci: Rename RUN_CI_ON_HOST to DANGER_RUN_CI_ON_HOST (Hennadii Stepanov)
97ba77aa8e58c3c7ff767e6ca804d0e46370e5dc ci: Add native s390x (Hennadii Stepanov)
Pull request description:
Unlike the Docker wrapped solution (#17591) this PR suggests running on host system directly.
This approach makes builds quick and stable (see: #18106).
The excerpt from the Travis log:
```
...
Running on host system without docker wrapper
...
Byte Order: Big Endian
...
```
ACKs for top commit:
MarcoFalke:
ACK 6136a96cdf631f824fb89cf2720422787439acc9
Tree-SHA512: 1b591de13e38d10a35217e1de11cbd648a359d18d16eed166fac18ea5788b58cc9fc6d407086ed342b99e57e479efd951a0ea693710177e500eb116316b9a788
fa7af33b4cb12315de86315c163393bdb50fad75 ci: Run unit tests sequential once (MarcoFalke)
fa68a3e7640fc00c47f03fa565357679bdafb4b2 appveyor: Enable minimal unit test logging to aid debugging (MarcoFalke)
Pull request description:
Fixes#16976
Top commit has no ACKs.
Tree-SHA512: 1f1ee8776a67afa8c1c5a16ef170c9975b6a486c087c8eba12e97d23382befd1c2801622ec70ca8e4cd1fbedce1dec46be67677ceaf07f35f1d3f3bead0200f0
faa9491870bf29583a989c407694d01f49b65bec ci: Use Focal for fuzzers (MarcoFalke)
Pull request description:
This gives us access to clang-10, as well as a newer version of valgrind
ACKs for top commit:
fanquake:
ACK faa9491870bf29583a989c407694d01f49b65bec - [Clang 10](https://packages.ubuntu.com/focal/clang) and [valgrind 3.15](https://packages.ubuntu.com/focal/valgrind).
practicalswift:
ACK faa9491870bf29583a989c407694d01f49b65bec -- diff looks correct & contemporary clang is better than vintage clang
Tree-SHA512: 0e67232673434c0309db79c1054e3e981115083585945967e346f4d58792635832100f89911428aab928155e44e5f401207a023681ae008fdb5280cf02c4d427
25c8b73656c90f7b78a90f25c5bcc308f8b7c598 ci: Use Homebrew addon on native macOS (Hennadii Stepanov)
596c627a1eb2b32e2755ae5b9bf32235cf8ce72b ci: Fix brew in Travis (Hennadii Stepanov)
Pull request description:
Recently almost every macOS image update on Travis breaks our builds:
- #17848
- #18436
This PR:
- fixes the error caused by the recent [update](https://changelog.travis-ci.com/xcode-11-3-1-xcode-11-2-1-xcode-11-1-and-xcode11-images-updated-142286) from 10.14.4 (18E226) to 10.14.6 (18G3020) on March 25
- leverages [Homebrew addon](https://config.travis-ci.com/ref/job/addons/homebrew) to install packages
Homebrew is not told to install `automake` and `pkg-config` packages, as the [docs](https://docs.travis-ci.com/user/reference/osx/#compilers-and-build-toolchain) states that they are pre-installed:
> - automake 1.16.1
> - pkg-config 0.29.2
Top commit has no ACKs.
Tree-SHA512: 1a70c06468fbe162503081b03dcf54614d67abf8ff0ce07d118b5bb50bbb92c182a76f769bea586c691aa82b9281a29cdef88091acc16895817a2e7cddafec6e
fae1e992898aa6b36c402cec4085fbf6da9b33ad ci: Only clone bitcoin-core/qa-assets when fuzzing (MarcoFalke)
Pull request description:
Currently the only content of that repo are some seeds, so we can speed up some ci builds
ACKs for top commit:
laanwj:
ACK fae1e992898aa6b36c402cec4085fbf6da9b33ad (provided this passes travis)
Tree-SHA512: ed813738e7f24bb56a2f12aa3b398e414eb4f0ba98379836a33ff3e5602cbf42a28e89aad10e346468191ecddc03e60d5b236097112e27c07cb1c2293533ea58
fa082d0a57afedca9122fac4aecd6a3070f06b04 travis: Remove valgrind (MarcoFalke)
Pull request description:
When the valgrind run was added, it took 2 hours. Travis kindly raised the timeout limit to the maximum possible of 3 hours.
Today, a full build of Bitcoin Core with all tests takes more than three hours. Thus, it is impossible to run all tests on travis.
Moreover, the feedback loop for developers that create a pull request takes at least 2 hours, but in some cases (when the travis queue is full) until the next day. This is unacceptable.
Fix both issues by removing the build from travis.
Please note that the `ci/test/` configurations are *not* removed. They will stay in the repo and can be executed anywhere (just not on travis).
ACKs for top commit:
jamesob:
ACK fa082d0a57
jnewbery:
utACK fa082d0a57afedca9122fac4aecd6a3070f06b04
Tree-SHA512: 9acaa0e2d3926014fadb7dd2e86c4e01df382e9399f6ae99f989fa609da66a77bdd1b75d6ff42d2686f38f730b8564e6dc722aa597a473290c9d30c2abe7ef0f
fab7d954b261d74b369fe2a3c1785540c4f056b4 test: Make valgrind.supp work on aarch64 (MarcoFalke)
Pull request description:
Was easy to fix by simply removing a line
ACKs for top commit:
practicalswift:
ACK fab7d954b261d74b369fe2a3c1785540c4f056b4 -- diff looks correct
Tree-SHA512: d2d7c6cac453a3177c20e256ec50a03066f8dbf5ae45299077ccf4a2b45a3a40252b1b5fcaf9224a59bb5c3df5bd90ac58af27eb0f47dc87c2640df5b2b460ca
fa92af5af39a08982f785542df5419d6d5a4706d ci: Run feature_block and feature_abortnode in valgrind (MarcoFalke)
fa01febeaf801bade77a613e64f18b556ae16d86 test: Remove ci timeout restriction in test_runner (MarcoFalke)
Pull request description:
Also revert commit 0a4912e46a, because some tests take too long for this to be useful anymore.
Top commit has no ACKs.
Tree-SHA512: 363f14766e1f4a5860ab668a516b41acebc6fbdf11d8defb3a95a772dbf82304ca1f5f14b1dbad97f2029503e03d92e8c69df0466a8872409c20665838f617ed
4444edc2e6671d3f73de3725447130f73ecf0375 ci: Enable all functional tests in valgrind (MarcoFalke)
Pull request description:
The travis timeout for our repo has been bumped to 2h, so we can run all tests in valgrind now
ACKs for top commit:
practicalswift:
ACK 4444edc2e6671d3f73de3725447130f73ecf0375 -- regarding the three disabled cases (`feature_abortnode`, `feature_block` and `rpc_bind`): not a big deal since MSan will take care of those once #18288 is merged. More is more :)
Tree-SHA512: ea2f798112911b6d1f3d88cfcdf0a7cdb698687248343703d6fe55da144542c961c15d888bffb41672c10aa76765615cb7c7ff93d468bfad3c51f962f24e7abb
## Issue being fixed or feature implemented
current develop fails to reindex whenever there is an issue at node
start (prints `should not be overwriting a chainstate` in `debug.log`)
## What was done?
reset chainman to allow it re-initialize chainstate
## How Has This Been Tested?
simulated an issue with
```
if (!fReset) {
strLoadError = _("DEBUG");
break;
}
```
## Breaking Changes
should not be any but pls test
## 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)_
## Issue being fixed or feature implemented
We should not be running jobs if their ancestors failed. This was broken
with the introduction of `FAST_MODE`/`.skip-in-fast-mode-template` I
think (via #3635).
## What was done?
Adjust ci rules, make them explicit.
## How Has This Been Tested?
Compare `test` stages:
https://gitlab.com/dashpay/dash/-/pipelines/888510672 (note: some of
`test` stage jobs were started, even though all related jobs from
`build` stage have failed)
vs
https://gitlab.com/UdjinM6/dash/-/pipelines/888923382 (note: none of
`test` stage jobs were started)
## 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)_
## Issue being fixed or feature implemented
Member obj.keyIDOwner is read & write twice
## What was done?
Fixed: it is serialized once
## How Has This Been Tested?
Unit/functional tests in CI
## Breaking Changes
Data format in database changed in incompatible way
## 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
## Issue being fixed or feature implemented
same as #5392, alternative solution
~based on #5402 atm, will rebase later~
## What was done?
pls see individual commits
## How Has This Been Tested?
reorg mainnet around forkpoint with a patched client (to allow low
difficulty), run tests
## Breaking Changes
Another evodb migration is required. Going back to an older version or
migrating after the fork requires reindexing.
## Checklist:
- [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)_
## Issue being fixed or feature implemented
Many objects created and functions called by passing `const
std::unique_ptr<Obj>& obj` instead directly passing `Obj& obj`
In some cases it is indeed needed, but in most cases it is just extra
complexity that is better to avoid.
Motivation:
- providing reference to object instead `unique_ptr` is giving warranty
that there's no `nullptr` and no need to keep it in mind
- value inside unique_ptr by reference can be changed externally and
instead `nullptr` it can turn to real object later (or in opposite)
- code is shorter but cleaner
Based on that this refactoring is useful as it reduces mental load when
reading or writing code.
`std::unique` should be used ONLY for owning object, but not for passing
it everywhere.
## What was done?
Replaced most of usages `std::unique_ptr<Obj>& obj` to `Obj& obj`.
Btw, in several cases implementation assumes that object can be nullptr
and replacement to reference is not possible.
Even using raw pointer is not possible, because the empty
std::unique_ptr can be initialized later somewhere in code.
For example, in `src/init.cpp` there's called `PeerManager::make` and
pass unique_ptr to the `node.llmq_ctx` that would be initialized way
later.
That is out of scope this PR.
List of cases, where reference to `std::unique_ptr` stayed as they are:
- `std::unique_ptr<LLMQContext>& llmq_ctx` in `PeerManagerImpl`,
`PeerManager` and `CDSNotificationInterface`
- `std::unique_ptr<CDeterministicMNManager>& dmnman` in
`CDSNotificationInterface`
Also `CChainState` have 3 references to `unique_ptr` that can't be
replaced too:
- `std::unique_ptr<llmq::CChainLocksHandler>& m_clhandler;`
- `std::unique_ptr<llmq::CInstantSendManager>& m_isman;`
- `std::unique_ptr<llmq::CQuorumBlockProcessor>&
m_quorum_block_processor;`
## How Has This Been Tested?
Run unit/functional tests.
## Breaking Changes
No breaking changes, all of these changes - are internal APIs for Dash
Core developers only.
## 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
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
1ef28b4f7cfba410fef524def1dac24bbc4086ca Make AnalyzePSBT next role calculation simple, correct (Gregory Sanders)
Pull request description:
Sniped test and alternative to https://github.com/bitcoin/bitcoin/pull/18220
Sjors documenting the issue:
```
A PSBT signed by ColdCard was analyzed as follows (see #17509 (comment))
{
"inputs": [
{
"has_utxo": true,
"is_final": false,
"next": "finalizer"
}
],
"estimated_vsize": 141,
"estimated_feerate": 1e-05,
"fee": 1.41e-06,
"next": "signer"
}
I changed AnalyzePSBT so that it returns "next": "finalizer" instead.
```
It makes it much clearer that the role has been decided before hitting the `calc_fee` block, and groups all state-deciding in one spot instead of 2.
Note that this assumes that PSBT roles are a complete ordering, which for now and in the future seems to be a correct assumption.
ACKs for top commit:
Sjors:
ACK 1ef28b4f7cfba410fef524def1dac24bbc4086ca, much nicer. Don't forget to document the bug fix.
achow101:
ACK 1ef28b4f7cfba410fef524def1dac24bbc4086ca
Empact:
ACK 1ef28b4f7c
Tree-SHA512: 22ba4234985c6f9c1445b14565c71268cfaa121c4ef000ee3d5117212b09442dee8d46d9701bceddaf355263fe25dfe40def2ef614d4f2fe66c9ce876cb49934
