Skip to content

backport: trivial backports 2023 04 09#5310

Merged
PastaPastaPasta merged 15 commits into
dashpay:developfrom
PastaPastaPasta:develop-trivial-2023-04-09
Apr 15, 2023
Merged

backport: trivial backports 2023 04 09#5310
PastaPastaPasta merged 15 commits into
dashpay:developfrom
PastaPastaPasta:develop-trivial-2023-04-09

Conversation

@PastaPastaPasta

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

Script cherry-picked backports from April 9th

What was done?

How Has This Been Tested?

make -j8 check; functional tests; quick overview; likely that fuzz build fails

Breaking Changes

should be none

Checklist:

  • 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

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@PastaPastaPasta PastaPastaPasta added this to the 20 milestone Apr 10, 2023
@PastaPastaPasta
PastaPastaPasta requested review from UdjinM6 and knst April 10, 2023 15:30

@knst knst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see a comment for get_machine

+CI failed in fuzz's code

Comment thread contrib/devtools/test-symbol-check.py Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's used below:

if 'riscv' in get_machine(cc):

Should be done after "Merge bitcoin#21255: contrib: run test-symbol-check for RISC-V"

@knst knst mentioned this pull request Apr 13, 2023
5 tasks
fanquake and others added 15 commits April 14, 2023 23:34
9086e0d build: set Unicode true for NSIS installer (fanquake)

Pull request description:

  Now that we are using Focal for gitian builds, and have [NSIS 3.0+ available](https://packages.ubuntu.com/focal/nsis) (also in Guix), we can create installers that [support unicode](https://nsis.sourceforge.io/Docs/Chapter4.html#aunicodetarget).

  Unicode is only becoming the NSIS default [beginning with the 3.07 release](https://nsis.sourceforge.io/Docs/AppendixF.html#v3.07-cl), so we need to set this attribute to get support.

  Should close: bitcoin#13817

  Gitian builds:
  ```bash
  b8553615b6b4be5e4459e03796e700b30b5d198a7f184f27be6983ff901b5592  bitcoin-9086e0dd3c92-win-unsigned.tar.gz
  a6b024a5a68e0196e8e118168c918285e820f2d0ffe9c38db680580459da8bf3  bitcoin-9086e0dd3c92-win64-debug.zip
  ff4003d4f61127c707e44b5235eaf924b30351f20cde27e775131982a1b4cf92  bitcoin-9086e0dd3c92-win64-setup-unsigned.exe
  1876bee55fa9ea99b91203975c13d0ad8a046b4b58068bde41c977fd1d12de13  bitcoin-9086e0dd3c92-win64.zip
  000f2778f8f166a89b4ab35f155156c1c34800be6e47d29b5308043c50128392  src/bitcoin-9086e0dd3c92.tar.gz
  d650a9b8f2dd1df777bf42439dfcbcf6bc358e30ec148b9992a18b39f76b1ecf  bitcoin-core-win-22-res.yml
  ```

ACKs for top commit:
  laanwj:
    ACK 9086e0d
  hebasto:
    ACK 9086e0d, tested on Windows 10 Pro (20H2, build 19042.804):

Tree-SHA512: cc7b7ca05877571d0a29a7d36e40279f54d886d8ab27facfa722c2ee95a1fc06c2bad8ef1eb1980d283ae981659d737021a46c8f4618e24d510b5ab384990e09
3f36468 fuzz: Avoid -Wreturn-type warnings (practicalswift)

Pull request description:

  Avoid `-Wreturn-type` warnings.

  Closes bitcoin#21355.

ACKs for top commit:
  MarcoFalke:
    cr ACK 3f36468
  fanquake:
    ACK 3f36468 - thanks for cleaning this up.

Tree-SHA512: 6fa2640a26e64d2bea60e016ad14b5c434137fedc0b3bf2ac244f02f9b1cd303d1ebac4ac4e6791534560f8311c4cbe9395c2ce94d7ec022d3b192f1ea070809
36aa295 fuzz: fix gcc Woverloaded-virtual build warnings (Jon Atack)

Pull request description:

  Possible fixup to gcc build warnings since merge of b22d4c1. Closes bitcoin#21369.

ACKs for top commit:
  practicalswift:
    cr ACK 36aa295: patch looks correct
  achow101:
    ACK 36aa295
  kristapsk:
    ACK 36aa295, this fixes compiler warnings for me with GCC 9.3.0.

Tree-SHA512: b6c99690ff72b809ce8105696744546252691b618f54311a9d930d9975fc692071ef408450f618fbb4aa99ee5390028a6eabbc968e22b2e8d2bd56bbafef49f8
…work queue exceeded

8dd5946 add functional test (Larry Ruane)
b5a80fa util: Handle HTTP_SERVICE_UNAVAILABLE in bitcoin-cli (Hennadii Stepanov)

Pull request description:

  If `bitcoind` is processing 16 RPC requests, attempting to submit another request using `bitcoin-cli` produces this less-than-helpful error message: `error: couldn't parse reply from server`. This PR changes the error to: `error: server response: Work queue depth exceeded`.

ACKs for top commit:
  fjahr:
    tACK 8dd5946
  luke-jr:
    utACK 8dd5946 (no changes since previous utACK)
  hebasto:
    re-ACK 8dd5946, only suggested changes since my [previous](bitcoin#18335 (review)) review.
  darosior:
    ACK 8dd5946

Tree-SHA512: 33e25f6ff05d9b56fae2bdb68b132557bb8e995f5438ac4fbbc53c304c5152a98aa43c43600c31d8a6a2830cbd48bf8ec7d89dce50190b29ec00a43830126913
ae9d26a wallet: Fix already-loading error message grammar (Fotis Koutoupas)

Pull request description:

ACKs for top commit:
  practicalswift:
    cr ACK ae9d26a
  prayank23:
    ACK bitcoin@ae9d26a

Tree-SHA512: 2f58d309dd33954f47e3ed339887b11bfdad4519f89ed3dd9bf3fb0db246d78b89653d553d02350ec84ce68bbb904db9fac00a2aad8d37f48df694d6782f35df
3d086f4 test: add ParseUInt16() test coverage (Jon Atack)

Pull request description:

  `ParseUInt16()` was just added in bitcoin#21328.

ACKs for top commit:
  practicalswift:
    cr ACK 3d086f4: patch looks correct & more coverage is better than less coverage

Tree-SHA512: bf7f96deb7c1531419565907f0ea8a8e32b368d4b823a3e80928b2c118edbf643ea06e357b4b5504a89f855caeed289daa9f823c740231ed6ad1b8ed00285ce8
7e34448 test: remove duplicate assertions in util_tests (Jon Atack)

Pull request description:

  as noticed by Kiminuo in bitcoin#21488 (comment)

ACKs for top commit:
  practicalswift:
    cr ACK 7e34448: patch looks correct
  vasild:
    ACK 7e34448

Tree-SHA512: ad3d5983ad3a665155d766843dfda7178ced47e82154838331e428ed0828a467c1cf4bf99270aaf191e94156d485fafd0a7d5bc68248c4c1304a00ca5a2a9d2e
5294f0d refactor: return std::nullopt instead of {} (fanquake)

Pull request description:

  In bitcoin#21415 [we decided](bitcoin#21415 (comment)) to return `std::optional` rather than `{}` for
  uninitialized values. This PR replaces the two remaining usages of `{}`
  with `std::nullopt`.

  As a side-effect, this also quells the spurious GCC 10.2.x warning that
  we've had reported quite a few times. i.e bitcoin#21318, bitcoin#21248, bitcoin#20797.

  ```bash
  txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’:
  txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
    898 |     return {};
        |             ^
  ```

ACKs for top commit:
  hebasto:
    ACK 5294f0d, I have reviewed the code and it looks OK, I agree it can be merged.

Tree-SHA512: 5b776be79ab26e5a3a5fc2b463b394ea5ce6797ed5558424873fa4ecee2898170eff76d6da9d69394d28f8f98974117fc63b922a3e19c52f5294c83073e79bb0
…gument

804ac10 remove unnecessary newline from initWarning() argument (Larry Ruane)

Pull request description:

  Run: `src/bitcoind -wallet=nosuchfile`

  Without this patch, `debug.log` contains:
  ```
  2021-03-23T21:19:16Z init message: Verifying wallet(s)...
  2021-03-23T21:19:16Z Warning: Skipping -wallet path that doesn't exist. Failed to load database path '/home/larry/.bitcoin/wallets/nosuchfile'. Path does not exist.

  2021-03-23T21:19:16Z init message: Loading banlist...
  ```
  With this patch, the empty line isn't present. This PR fixes a similar problem with `src/bitcoind -conf=nosuchfile`

ACKs for top commit:
  practicalswift:
    cr ACK 804ac10: patch looks correct!
  jarolrod:
    tACK 804ac10, nice catch!
  theStack:
    Code-review ACK 804ac10

Tree-SHA512: dfcbaaa72ca24ac40233ac56840cfba8827853711d3df6e229ce940686f2ebf8bf0560bafcaa73a4d82d179a5050af0d3cabdc47b3b1dfd6aaadf718a6635f11
…rialization [ZAP1]

83a425d compressor: use a prevector in compressed script serialization (William Casarin)

Pull request description:

  This function was doing millions of unnecessary heap allocations during IBD.

  I'm start to catalog unnecessary heap allocations as a pet project of mine: as-zero-as-possible-alloc IBD. This is one small step.

  before:
  ![May01-174536](https://user-images.githubusercontent.com/45598/80850964-9a38de80-8bd3-11ea-8eec-08cd38ee1fa1.png)

  after:
  ![May01-174610](https://user-images.githubusercontent.com/45598/80850974-a91f9100-8bd3-11ea-94a1-e2077391f6f4.png)

  ~should I type alias this?~ *I type aliased it*

  This is a part of the Zero Allocations Project bitcoin#18849 (ZAP1). This code came up as a place where many allocations occur.

ACKs for top commit:
  Empact:
    ACK bitcoin@83a425d
  elichai:
    tACK 83a425d
  sipa:
    utACK 83a425d

Tree-SHA512: f0ffa6ab0ea1632715b0b76362753f9f6935f05cdcc80d85566774401155a3c57ad45a687942a1806d3503858f0bb698da9243746c8e2edb8fdf13611235b0e0
fa872c9 depends: Fix id_string invocations (Carl Dong)

Pull request description:

  Closes: bitcoin#21242

  ```
  Reproduced from depends/Makefile comment:

  When invoking a shell, GNU Make special-cases exit code 127 (command not
  found) by not capturing the output but instead passing it through. This
  is not done for any other exit code.

  Therefore, we require a "|| true" to avoid this behaviour when in an
  environment where the build_* or host_* may not exist yet.
  ```

ACKs for top commit:
  laanwj:
    Concept and light code review ACK fa872c9

Tree-SHA512: 9ce88381aec579d956572cf70c4f69dc5a3873f0d2af14a71cf24814192a89452b8280258bed8cca804e4bd2644db056d213ab733df46a10560a47079524d8ac
… to fail

facfc0f fuzz: Remove strprintf test cases that are known to fail (MarcoFalke)

Pull request description:

  They are still waiting to be fixed (see c42f/tinyformat#70 ), so no need for us to carry them around in our source code. They can be added back once upstream is fixed.

  Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34082

ACKs for top commit:
  laanwj:
    Code review ACK facfc0f

Tree-SHA512: d9d3d35555b6d58740a041ae45797ca85149f60990e2ed632c5dadf363e1d2362d2447681d7ceaa1fbffcd6e7bc8da5bc15d3923b68829a86c25b364a599afc8
fa95555 fuzz: Limit max insertions in timedata fuzz test (MarcoFalke)

Pull request description:

  It is debatable whether a size of the median filter other than `200` (the only size used in production) should be fuzzed. For now add a minimal patch to cap the max insertions. Otherwise the complexity is N^2 log(N), where N is the size of the fuzz input.

  Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34167

ACKs for top commit:
  practicalswift:
    cr ACK fa95555: patch looks correct

Tree-SHA512: be7737e9f4c906053e355641de84dde31fed37ed6be4c5e92e602ca7675dffdaf06b7063b9235ef541b05d3d5fd689c99479317473bb15cb5271b8baabffd0f2
9c891b6 net: initialize nMessageSize to max uint32_t instead of -1 (eugene)

Pull request description:

  nMessageSize is uint32_t and is set to -1. This will warn with `-fsanitize=implicit-integer-sign-change` when V1TransportDeserializer calls into the ctor.  This pull initializes nMessageSize to `numeric_limits<uint32_t>::max()` instead and removes the ubsan suppression.

ACKs for top commit:
  laanwj:
    Code review ACK 9c891b6
  promag:
    Code review ACK 9c891b6.

Tree-SHA512: f05173d9553a01d207a5a7f8ff113d9e11354c50b494a67d44d3931c151581599a9da4e28f40edd113f4698ea9115e6092b2a5b7329c841426726772076c1493
…error while accessing it

29c9e2c wallet: Do not iterate a directory if having an error while accessing it (Hennadii Stepanov)

Pull request description:

  On Windows when `ListDatabases` tries to iterate any system folder, e.g., "System Volume Information", it falls into an infinite loop.

  This PR fixes this bug. Now the `debug.log` contains:
  ```
  2021-05-12T09:07:53Z ListDatabases: Access is denied D:/System Volume Information -- skipping.
  ```

  An easy way to reproduce the bug and test this PR is to pass the `-walletdir=D:\` command-line option, and run the `listwalletdir` RPC, or File -> Open Wallet in the GUI menu.

  Fixes bitcoin#20081.
  Fixes bitcoin#21136.
  Fixes bitcoin#21904.

  Also https://bitcoin.stackexchange.com/questions/99243/listwalletdir-access-is-denied-d-system-volume-information

ACKs for top commit:
  prayank23:
    ACK bitcoin@29c9e2c
  promag:
    Code review ACK 29c9e2c.
  meshcollider:
    Code review ACK 29c9e2c

Tree-SHA512: b851c88e6d09626f4cb81acc2fa59a563b2aee64582963285715bf785c64b872e8bf738aa6b27bdbaf4c3e5c8565c2dc2c802135f9aa1f48b4b913435bc5d793
@PastaPastaPasta
PastaPastaPasta force-pushed the develop-trivial-2023-04-09 branch from cd47c6b to dc1f37e Compare April 15, 2023 04:34
@PastaPastaPasta
PastaPastaPasta requested a review from knst April 15, 2023 04:34
@PastaPastaPasta

Copy link
Copy Markdown
Member Author

All issues should be resolved :)

@UdjinM6 UdjinM6 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, utACK

@PastaPastaPasta
PastaPastaPasta merged commit 49e998f into dashpay:develop Apr 15, 2023
@PastaPastaPasta
PastaPastaPasta deleted the develop-trivial-2023-04-09 branch April 15, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants