Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

backport: trivial 2024 10 23 pr7 #6350

Merged
merged 20 commits into from
Oct 25, 2024
Merged

backport: trivial 2024 10 23 pr7 #6350

merged 20 commits into from
Oct 25, 2024

Conversation

PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

Batch of trivial backports

What was done?

See commits

How Has This Been Tested?

built locally; large combined merge passed tests locally

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
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@fanquake @PastaPastaPasta
Merge bitcoin#27328: depends: fix osx build with clang 16
0a94b3f 
87afcb0 depends: fix osx build with clang 16 (Cory Fields)

Pull request description:

  Current build (using forced system clang as a test) results in:

  > error: unknown argument: '-internal-externc-isystem/opt/clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/include'

  For some reason the previous syntax worked with clang 15 and below, but clang 16 requires that the option and value are properly separated.

  See [here for an example of upstream using this syntax](https://github.com/llvm/llvm-project/blob/main/clang/test/Driver/crash-report-with-asserts.c#L9).

  There is no change in behavior for previous versions.

  I'm seeing an additional unrelated problem with linking with system clang, but I'll PR the solution to that separately as it's not as straightforward as this.

ACKs for top commit:
  TheCharlatan:
    ACK 87afcb0
  hebasto:
    ACK 87afcb0

Tree-SHA512: 127037c888c37c6ccd9679e96da34037cc43ccdc07915865a0a5494edb62633e83fc1bd6b1c4bb7a0322f5b59622e10090a31987f38496fb6b306488e9941594
@fanquake @PastaPastaPasta
Merge bitcoin#27301: depends: make fontconfig build under clang-16
a11690b 
9cbc1c2 depends: make fontconfig build under clang-16 (fanquake)

Pull request description:

  Use the same workaround we've applied to qrencode, and other packages. Fontconfig not building is currently a blocker for fuzz/sanitizer infra upgrades (bitcoin#27298).

  For now, this is also more straightforward than bumping the package, which introduces more complexity/usage of gperf.

  Closes: bitcoin#27299.

ACKs for top commit:
  hebasto:
    ACK 9cbc1c2

Tree-SHA512: 387ea1a73e3429f166ef5278305a56cb3c69b6e3fc8a21a66521738e313e3fe783f042759b396cd88e28c10918a4427fb836a8dfecc5a846723b6f6c6a7ade51
@fanquake @PastaPastaPasta
Merge bitcoin#27368: refactor: Drop no longer used CNetMsgMaker ins…
671e8e6 
…tances

ea7ec78 refactor: Drop no longer used `CNetMsgMaker` instances (Hennadii Stepanov)

Pull request description:

  The removed lines have been unused since the bitcoin@abf5d16 commit from bitcoin#25454.

ACKs for top commit:
  dergoegge:
    utACK ea7ec78
  Sjors:
    ACK ea7ec78
  TheCharlatan:
    ACK ea7ec78

Tree-SHA512: 9a2a9ff3f124b68a8cd20a637e90885096996c3aa354a4d8adbec98f5761e9e826c1c064ccd90aaf6d72beac61dd9e22c8b76d089e18bba6e0ad51e59a9c7df8
@fanquake @PastaPastaPasta
Merge bitcoin#27362: test: remove GetRNGState lsan suppression
5645362 
71b3e9b sanitizers: remove GetRNGState lsan suppression (fanquake)

Pull request description:

  I am no-longer seeing this, testing with the native_asan job over `x86_64` (Ubuntu 22.04) and `aarch64` (Fedora 37).

  Can anyone recreate the false-positive?

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 71b3e9b
  hebasto:
    ACK 71b3e9b, tested on Ubuntu 22.04 x86_64.

Tree-SHA512: 63020327d61acd6c94c6c278c9c4d72aedc10253fa172bcf9353bcad4c28d068bee824969eb3ce92152244831df8fe92cffae536453c8073a4fda74dfdfbcefa
@fanquake @PastaPastaPasta
Merge bitcoin#26741: doc: FreeBSD DataDirectoryGroupReadable Setting
07770b7 
499c464 doc: update DataDirectoryGroupReadable 1 in tor.md (Jesse Barton)

Pull request description:

  Updating tor.md doc to include mention of FreeBSD requiring the DataDirectoryGroupReadable be set to 1.
  Default per the FreeBSD man page is 0.

         DataDirectoryGroupReadable 0|1
     If this option is set to 0, don't allow the filesystem groupto
     readthe DataDirectory. If the option is setto 1, make the
     DataDirectory readable by the default GID. (Default:0)

ACKs for top commit:
  vasild:
    ACK 499c464

Tree-SHA512: 8750b49cd04e900435c7991d1a24641fd1171227c1f14ed59afb157f24c1ca60380d30aecfb174ca46fd5b4b99dcdb3a1cfd019aafc343362e8103abf7c17e6a
@fanquake @PastaPastaPasta
Merge bitcoin#27447: depends: Remove _LIBCPP_DEBUG from depends DEB…
9b58b2d 
…UG mode

bc4fd49 depends: add _LIBCPP_ENABLE_ASSERTIONS to DEBUG mode (fanquake)
cf266b2 depends: Remove _LIBCPP_DEBUG from depends DEBUG mode (fanquake)

Pull request description:

  It was deprecated in LLVM 15, turned into compile-time error in LLVM 16:
  ```bash
  In file included from /usr/lib/llvm-16/bin/../include/c++/v1/cassert:19:
  /usr/lib/llvm-16/bin/../include/c++/v1/__assert:22:5: error: "Defining _LIBCPP_DEBUG is not supported anymore.
  Please use _LIBCPP_ENABLE_DEBUG_MODE instead."
      ^
  1 error generated.
  ```

  and has been removed entirely in LLVM 17 (main): llvm/llvm-project@ff573a4.

  [Building libc++ in debug mode](https://releases.llvm.org/16.0.0/projects/libcxx/docs/DesignDocs/DebugMode.html), will also automatically set
  `_LIBCPP_ENABLE_DEBUG_MODE` (the new define), so adding it to depends
  doesn't seem useful, and would just result in redefinition errors.

  I'm wondering if as a followup, we could enable a DEBUG build of libc++
  in our MSAN CI job? i.e https://github.com/fanquake/bitcoin/tree/msan_with_enable_debug_mode.

  Somewhat related to google/oss-fuzz#9828, where
  it looks like we'll have to sort out getting a DEBUG build of LLVM, and can drop the commentary about re-enabling DEBUG=1.

ACKs for top commit:
  MarcoFalke:
    lgtm Approach ACK bc4fd49

Tree-SHA512: 9c0f48fc428278fbf34fbb8f81e761e232506d7ab28e971cb9a9b9a81d549b4d8bbe51e2f7608d56e489428679231da5b7431443849b238a8a993ad241740282
@PastaPastaPasta PastaPastaPasta added this to the 22 milestone Oct 24, 2024
UdjinM6
UdjinM6 requested changes Oct 25, 2024
View reviewed changes
src/test/fuzz/rpc.cpp Outdated Show resolved Hide resolved
fanquake and others added 14 commits October 25, 2024 09:59
@fanquake @PastaPastaPasta
Merge bitcoin#27506: test: prevent intermittent failures
df7be02 
10a354f test: prevent intermittent failures (Amiti Uttarwar)

Pull request description:

  Follow up to bitcoin#27214 - add an address to the tried table before the new table to make sure a new table collision is not possible.

ACKs for top commit:
  mzumsande:
    Code review ACK 10a354f - the fix is what I suggested [here](bitcoin#27214 (comment)) and should make these intermittent failures impossible.

Tree-SHA512: 24099f02e1915395130065af0ef6a2a1893955d222517d156d928765541d9c427da00172a9b5a540163f4d6aae93ca3882e8267eeb35ecc595d42178abc6191c
@fanquake @PastaPastaPasta
Merge bitcoin#27508: build: use latest config.{guess,sub} in depends
90d65f2 
4a3f1db depends: latest config.sub (fanquake)
ac462c5 depends: latest config.guess (fanquake)

Pull request description:

  Been a few years since we last updated these.
  Also related to bitcoin#26422 (comment).

ACKs for top commit:
  TheCharlatan:
    ACK 4a3f1db
  hebasto:
    ACK 4a3f1db, I've got zero diff with files from the [upstream](https://git.savannah.gnu.org/gitweb/?p=config.git;a=tree).

Tree-SHA512: 8f1af0813c56289c796a6e74965632dd6fa6dd135409250b2d5ebf7c1c2bfb4001195d35e5d7ecc0cad2a049468193b9fefc2b26beb7669afe6bba4d9c3ffa33
@fanquake @PastaPastaPasta
Merge bitcoin#27516: test: simplify uint256 (de)serialization routines
712dcaf 
96bf0bc test: simplify uint256 (de)serialization routines (Sebastian Falbesoner)

Pull request description:

  These routines look fancy, but do nothing more than converting between byte objects of length 32 to/from integers in little endian byte order and can be replaced by simple one-liners, using the `int.{from,to}_bytes` methods (available since Python 3.2).

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 96bf0bc
  brunoerg:
    crACK 96bf0bc

Tree-SHA512: f3031502d61a936147867ad8a0efa841a9bbdd2acf8781653036889a38524f4f1a5c86b1e07157bf2d9663097e7b84be6846678d0883d2a334beafd87e9510f0
@fanquake @PastaPastaPasta
Merge bitcoin#25937: test: add coverage for rpc error when trying to …
3306f96 
…rescan beyond pruned data

cca4f82 test: add coverage for rpc error when trying to rescan beyond pruned data (brunoerg)

Pull request description:

  This PR adds test coverage for the following rpc error:
  https://github.com/bitcoin/bitcoin/blob/15692e2641592394bdd4da0a7c2d371de8e576dd/src/wallet/rpc/transactions.cpp#L896-L899

ACKs for top commit:
  MarcoFalke:
    lgtm ACK cca4f82
  aureleoules:
    ACK cca4f82

Tree-SHA512: 724a055e9f6cddf1935699e8769015115f24f6485a0bd87e8660072ee44a15c1bddfdda848acc101ea7184b7e65a33b5b0d80b563d2ba3ecdab7a631378d6476
@fanquake @PastaPastaPasta
Merge bitcoin#26314: test: perturb anchors.dat to test error during i…
f725ed5 
…nitialization

33fdfc7 test: perturb anchors.dat to test it doesn't throw an error during initialization (brunoerg)

Pull request description:

  Got some inspiration from `feature_init`. This PR tests whether perturbing `anchors.dat` doesn't throw any error during initialization.

  https://github.com/bitcoin/bitcoin/blob/3f1f5f6f1ec49d0fb2acfddec4021b3582ba0553/src/addrdb.cpp#L223-L235

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 33fdfc7

Tree-SHA512: e6584debb37647677581fda08366b45b42803022cc4c4f1d5a7bd5e9e04d64da77656dad2b804855337487bdcfc891f300a2e03668d6122de769dd14f39af9ed
@fanquake @PastaPastaPasta
Merge bitcoin#26604: test: add coverage for -bantime
deb7de2 
9c18992 test: add coverage for `-bantime` (brunoerg)

Pull request description:

  This PR adds test coverage for `-bantime`. This flag sets the time in seconds how long the IP is banned (in the case you don't explicitly set `bantime` when using `setban`).

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 9c18992

Tree-SHA512: e95f8608aa5df9b09cc5577daae662ed79ef5d5c69ee5e704d7c69520b9b51cc142e9e6be69d80356eda25a5215c4770b1a208638560c48cd3bc8f6d195a371f
@fanquake @PastaPastaPasta
Merge bitcoin#26953: contrib: add ELF OS ABI check to symbol-check.py
a4e429c 
65ba8a7 contrib: add ELF ABI check to symbol-check.py (fanquake)

Pull request description:

  Check that the operating system ABI version embedded into the release binaries, is the version we expect it to be.

ACKs for top commit:
  laanwj:
    Code review ACK 65ba8a7
  TheCharlatan:
    ACK 65ba8a7

Tree-SHA512: 798d7c3b05183becf113a2ea13d889e18f1cec01d3cc279e64dbddede4d57f87444978f3f52c44bc5fdf0ba93d77c7c0be37aa815f93f348c35da45dc3d30ac2
@glozow @PastaPastaPasta
Merge bitcoin#27559: doc: clarify processing of mempool-msgs when NOD…
898dcbd 
…E_BLOOM

4581a68 clarify processing of mempool-msgs when NODE_BLOOM (0xb10c)

Pull request description:

  Under which circumstances we process received 'mempool' P2P messages caused confusion in bitcoin#27426. Rather than bike-shedding the formulation of the IF-statement, this adds a comment clarifying when we process the message. Also, correcting the `m_send_mempool` description.

ACKs for top commit:
  dergoegge:
    ACK 4581a68
  willcl-ark:
    ACK 4581a68
  glozow:
    ACK 4581a68

Tree-SHA512: 51ec673c3446b67c26f6c715430d0708b998b256260f5f5d0c034f271be8447d0bb8540dfd3879aa51904512fb26c9411766786c86287acff62d037a1df88855
@fanquake @PastaPastaPasta
Merge bitcoin#27422: test: add coverage to rpc_scantxoutset.py
417f71a 
7e3d4f8 test: add coverage to ensure the first arg of scantxoutset is needed (ismaelsadeeq)

Pull request description:

  Include a test that checks whether the first argument of scantxoutset RPC call "start" is required.
  The rpc call should fail if the "start" argument is not provided.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 7e3d4f8

Tree-SHA512: 6a456af9f3ccd5437be2edcd61936eb9f9c21ab926a6056c2c11b6b5121d1caca4e1f2ffd09015f9414af152c635a20e1da041eefdef980afbe8a0e8ccce07bd
@fanquake @PastaPastaPasta
Merge bitcoin#26653: test, init: perturb file to ensure failure inste…
71c6d7f 
…ad of only deleting them

c371cae test, init: perturb file to ensure failure instead of only deleting them (brunoerg)

Pull request description:

  In `feature_init.py` there is a TODO about perturbing the files instead of only testing by deleting them.
  ```py
              # TODO: at some point, we should test perturbing the files instead of removing
              # them, e.g.
              #
              # contents = target_file.read_bytes()
              # tweaked_contents = bytearray(contents)
              # tweaked_contents[50:250] = b'1' * 200
              # target_file.write_bytes(bytes(tweaked_contents))
              #
              # At the moment I can't get this to work (bitcoind loads successfully?) so
              # investigate doing this later.
  ```

  This PR adds it by writing into the file random bytes and checking whether it throws an error when starting.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK c371cae

Tree-SHA512: d691eee60b91dd9d1b200588608f56b0a10dccd9761a75254b69e0ba5e5866cae14d2f90cb2bd7ec0f95b0617c2562cd33f20892ffd16355b6df770d3806a0ff
@fanquake @PastaPastaPasta
Merge bitcoin#27575: Introduce platform-agnostic ALWAYS_INLINE macro
b8ddcd9 
3f19875 scripted-diff: Use platform-agnostic `ALWAYS_INLINE` macro (Hennadii Stepanov)
e16c22f Introduce platform-agnostic `ALWAYS_INLINE` macro (Hennadii Stepanov)

Pull request description:

  Split from bitcoin#24773 as requested in bitcoin#24773 (comment).

ACKs for top commit:
  theuni:
    utACK 3f19875
  fanquake:
    ACK 3f19875

Tree-SHA512: a19b713433bb4d3c5fff1ddb4d1413837823a400c1d46363a8181e7632b059846ba92264be1c867f35f532af90945ed20887103471b09c07623e0f3905b4098b
@fanquake @PastaPastaPasta
Merge bitcoin#27628: build: Fix shared lib linking for darwin with lld
45cc44b 
67aacc7 build: cleanup comments after adding yet another libtool hack (Cory Fields)
283d955 build: Fix shared lib linking for darwin with lld (Cory Fields)

Pull request description:

  Solves one of the last remaining blockers for bitcoin#21778. Fixes lld linking shared libs for macos via libtool.

  lld fails one of libtool's earliest checks [because it happens to output a warning that contains a specific string](https://git.savannah.gnu.org/cgit/libtool.git/tree/m4/libtool.m4#n999):

  >     # If there is a non-empty error log, and "single_module"
  >     # appears in it, assume the flag caused a linker warning

  And here is the test being run:
  > x86_64-apple-darwin-ld: warning: Option `-single_module' is deprecated in ld64:
  > x86_64-apple-darwin-ld: warning: Unnecessary option: this is already the default

  Because the warning is printed the test fails. So libtool falls back to a very primitive and broken link-line for shared libs.

  Arguably this should be worked-around in upstream lld by changing the warning string, as otherwise every libtool project will fail to link with it.

  Like many other libtool hacks, the solution is to simply disable the check and hard-code the answer we know to be correct.

ACKs for top commit:
  hebasto:
    re-ACK 67aacc7

Tree-SHA512: 792e4d208a3a4921edb5f267f43ecd052b5b650df0db5cb2788ee1e4f3c4087413f354b22e407ff5fa2f99a22a16154ec6826d14c6654a57c00aae3b3e744bca
@fanquake @PastaPastaPasta
Merge bitcoin#28479: build: use _LIBCPP_ENABLE_DEBUG_MODE over ENABLE…
3239f15 
…_ASSERTIONS

4a82503 build: use _LIBCPP_ENABLE_DEBUG_MODE over ENABLE_ASSERTIONS (fanquake)

Pull request description:

  `_LIBCPP_ENABLE_ASSERTIONS` is deprecated, and will be removed. [See (from libc++ __config in main)](https://github.com/llvm/llvm-project/blob/b57df9fe9a1a230f277d671bfa0884bbda9fc1c5/libcxx/include/__config#L205-L209):

  > TODO(hardening): remove this in LLVM 19.
  > This is for backward compatibility -- make enabling `_LIBCPP_ENABLE_ASSERTIONS` (which predates hardening modes)
  > equivalent to setting the safe mode.
  > ifdef _LIBCPP_ENABLE_ASSERTIONS
  > warning "_LIBCPP_ENABLE_ASSERTIONS is deprecated, please use _LIBCPP_ENABLE_SAFE_MODE instead."

  From LLVM 17, `_LIBCPP_ENABLE_DEBUG_MODE` can be used instead, which also performs more checks than safe mode:

  > Enables the debug mode which contains all the checks from the hardened mode and additionally more expensive checks that may affect the complexity of algorithms. The debug mode is intended to be used for testing, not in production. Mutually exclusive with `_LIBCPP_ENABLE_HARDENED_MODE` and `_LIBCPP_ENABLE_SAFE_MODE`.

  See https://libcxx.llvm.org/Hardening.html.

  Related to bitcoin#28476.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 4a82503 🙏

Tree-SHA512: ca52603f86214e8e9350bd2b2baa44fbde0f72f1b186da7aecd8690256dff5b2be75fe89383158298a6f683bbd6ae0dff528d2ba4cc5ece1f56cfbdee0e1dc5d
@fanquake @PastaPastaPasta
Merge bitcoin#28781: depends: latest config.guess & config.sub
37389c7 
49a9257 build: latest config.sub in depends (fanquake)
ced0435 build: latest config.guess in depends (fanquake)

Pull request description:

  Before we make any local modifications (i.e bitcoin#28733) pull the latest files from upstream.

ACKs for top commit:
  TheCharlatan:
    ACK 49a9257

Tree-SHA512: fbbe0d6ef72a196a652467af0550b38da23b932fe68da4965a9b0dc4795db9c869969db98f660cd360f6af3a7659b46c25e3fd398e0ef127dae71726b9a915a6
@PastaPastaPasta PastaPastaPasta requested review from UdjinM6 and removed request for shumkov October 25, 2024 14:59
@PastaPastaPasta PastaPastaPasta removed the request for review from ogabrielides October 25, 2024 15:00
UdjinM6
UdjinM6 approved these changes Oct 25, 2024
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK 37389c7

kwvg
kwvg approved these changes Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@kwvg kwvg left a comment

Choose a reason for hiding this comment

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

utACK 37389c7

@PastaPastaPasta PastaPastaPasta merged commit e12afdf into dashpay:develop Oct 25, 2024
33 of 35 checks passed
@PastaPastaPasta PastaPastaPasta deleted the develop-trivial-2024-10-23-pr7 branch October 25, 2024 17:58
knst
knst reviewed Oct 25, 2024
View reviewed changes
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

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

post-utACK 37389c7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knst knst knst left review comments

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@kwvg kwvg kwvg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
22
Development

Successfully merging this pull request may close these issues.
6 participants
@PastaPastaPasta @knst @UdjinM6 @kwvg @fanquake @glozow