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.

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

backport: trivial backports bitcoin v21-v22 #5739

Merged
merged 28 commits into from
Dec 4, 2023

Conversation

knst
Copy link
Collaborator

@knst knst commented Nov 28, 2023

Issue being fixed or feature implemented

Trivial backports from bitcoin v21-v22

What was done?

How Has This Been Tested?

Run unit/functional tests

Breaking Changes

N/A

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)

@knst knst added this to the 20.1 milestone Nov 28, 2023
@knst knst marked this pull request as ready for review November 29, 2023 11:22
src/init.cpp Outdated Show resolved Hide resolved
@knst knst requested a review from UdjinM6 November 30, 2023 07:37
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

@UdjinM6
Copy link

UdjinM6 commented Nov 30, 2023

rebased cause gitlab ci just wouldn't start otherwise for some reason

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK for merging via merge commit

fanquake and others added 19 commits December 3, 2023 20:44
1e9bfd4 qt: Reset toolbar after all wallets are closed (Hennadii Stepanov)

Pull request description:

  If the last open wallet is closed from the non-"Overview" tab, that tab remains active when a new wallet is opened:

  ![Screenshot from 2020-05-06 09-00-26](https://user-images.githubusercontent.com/32963518/81142394-46821880-8f78-11ea-84b5-1d02f2b7f3f1.png)

  This PR fixes this bug.

ACKs for top commit:
  promag:
    Code review ACK 1e9bfd4.
  luke-jr:
    utACK 1e9bfd4
  jonasschnelli:
    utACK 1e9bfd4

Tree-SHA512: a8dfd7591267e9544ad40c87581d554e5cfaad4b2a5bbfdbaf2596dc6869d2ac6cf7877adfef3d528fc61b081d40c6e30d787bbd7280ef7946aa7f7d9bc8b18e
8989577 Fix length of R check in test/key_tests.cpp:key_signature_tests (Dmitry Petukhov)

Pull request description:

  The code before the fix only checked the length of R value of the last
  signature in the loop, and only for equality (but the length can be
  less than 32)

  The fixed code checks that length of the R value is less than or equal
  to 32 on each iteration of the loop

ACKs for top commit:
  laanwj:
    ACK 8989577

Tree-SHA512: 73eb2bb4a6c1c5fc11dd16851b28b43037ac06ef8cfc3b1f957429a1fca295c9422d67ec6c73c0e4bb4919f92e22dc5d733e84840b0d01ad1a529aa20d906ebf
…nstandard

efaf80e fuzz: check that certain script TxoutType are nonstandard (Michael Dietz)

Pull request description:

  - Every transaction of type NONSTANDARD must not be a standard script
  - The only know types of nonstandard scripts are NONSTANDARD and certain NULL_DATA and MULTISIG scripts

  When reviewing bitcoin#20761 I figured this is very similar and might also be good to have

ACKs for top commit:
  MarcoFalke:
    ACK efaf80e

Tree-SHA512: 6f563ee3104ea9d2633aad95f1d003474bea759d0f22636c37aa91b5536a6ff0800c42447285ca8ed12f1b3699bf781dae1e5e0a3362da578749cd3164a06ea4
90f9fc2 qt: Save QSplitter state in QSettings (Hennadii Stepanov)

Pull request description:

  This PR adds the ability to save the `QSplitter` widget state in `QSettings` during shutdown, and restore it on startup.

  A user no longer needs to adjust the splitter every time :)
  ![DeepinScreenshot_select-area_20201225211422](https://user-images.githubusercontent.com/32963518/103141024-046c3980-46f7-11eb-9a8c-83613527ffe1.png)

ACKs for top commit:
  jonasschnelli:
    utACK 90f9fc2
  jonatack:
    ACK 90f9fc2 this sets the "PeersTabSplitterSizes" value in the RPCConsole dtor and restores it in the RPCConsole ctor; tested in Debian with various split settings, tab open/close sequences, and shutdown methods, and the Peers window split state was faithfully maintained.

Tree-SHA512: efbd6a4cee512982944955d36775e75a8a217b1dc49e62d42c6e402d2710dd44324b2c3c1edeb5fe38d9229e0e4a39734d1f4e63405ade8694762e1bbf72020b
2a39ccf Add include for std::bind. (sinetek)

Pull request description:

  Hi, this patch adds in <functional> because the GUI code makes use of std::bind.
  That's all.

ACKs for top commit:
  jonasschnelli:
    utACK 2a39ccf

Tree-SHA512: fb5ac07d9cd5d006182b52857b289a9926362a2f1bfa4f7f1c78a088670e2ccf39ca28214781df82e8de3909fa3e69685fe1124a7e3ead758575839f5f2277a9
5353b0c Change type definitions for "chain" and "setup_clean_chain" from type comments to Python 3.6+ types. Additionally, set type for "nodes". (Kiminuo)

Pull request description:

  ### Motivation

  When I wanted to understand better https://github.com/bitcoin/bitcoin/pull/19145/files#diff-4bebbd3b112dc222ea7e75ef051838ceffcee63b9e9234a98a4cc7251d34451b test, I noticed that navigation in PyCharm/VS Code did not work for `nodes` variable. I think this is frustrating, especially for newcomers.

  ### Summary

  * This PR modifies Python 3.5 [type comments](https://mypy.readthedocs.io/en/stable/cheat_sheet_py3.html#variables) to Python 3.6+ types and adds a proper type for `nodes` [instance attribute](https://mypy.readthedocs.io/en/stable/class_basics.html#instance-and-class-attributes).
  * This PR does not change behavior.
  * This PR is intentionally very small, if the concept is accepted, a follow-up PRs can be more ambitious.

  ### End result

  1. Open `test/functional/feature_abortnode.py`
  2. Move your caret to: `self.nodes[0].generate[caret here](3)`
  3. Use "Go to definition" [F12] should work now.

  I have tested this on PyCharm (Windows, Ubuntu) and VS Code (Windows, Ubuntu).

  Note: Some `TestNode` methods (e.g. `self.nodes[0].getblock(...)` ) use `__call__` mechanism and navigation does not work for them even with this PR.

ACKs for top commit:
  laanwj:
    ACK 5353b0c
  theStack:
    ACK 5353b0c

Tree-SHA512: 821773f052ab9b2889dc357d38c59407a4af09e3b86d7134fcca7d78e5edf3a5ede9bfb37595ea97caf9ebfcbda372bcf73763b7f89b0677670f21b3e396a12b
…n […].dat" log message to debug category

25f899c log: Move "Pre-allocating up to position 0x[...] in [...].dat" log message to debug category (practicalswift)
acd7980 log: Move "Leaving block file [...]: [...]" log message to debug category (practicalswift)

Pull request description:

  Move `Pre-allocating up to position 0x[…] in […].dat` log message to debug category.

  After the cleanup of `-debug=net` log messages PR (bitcoin#20724) was merged recently the console log now has very high signal to noise ratio. That's great! :)

  This PR increases the signal to noise ratio slightly more by moving the most common remaining implementation detail log message (`Pre-allocating up to position 0x[…] in […].dat`) to the debug category where it belongs :)

  Expected standard output from `bitcoind` (when in steady state) before this patch:

  ```
  $ src/bitcoind
  …
  0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
  0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
  0000-00-00T00:00:00Z Pre-allocating up to position 0x0000000 in blk00000.dat
  0000-00-00T00:00:00Z Pre-allocating up to position 0x000000 in rev00000.dat
  0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
  0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
  0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
  ```

  Expected standard output from `bitcoind` (when in steady state) after this patch:

  ```
  $ src/bitcoind
  …
  0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
  0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
  0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
  0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
  0000-00-00T00:00:00Z UpdateTip: new best=0000000000000000000000000000000000000000000000000000000000000000 height=000000 version=0x00000000 log0_work=00.000000 tx=000000000 date='0000-00-00T00:00:00Z' progress=0.000000 cache=000.0MiB(0000000txo)
  ```

  I find the latter alternative much easier to visually scan for anomalies (and more aesthetically pleasing TBH!).

  Non-GUI users deserve nice interfaces too :)

ACKs for top commit:
  laanwj:
    ACK 25f899c

Tree-SHA512: 5970798c41b041527ebdcbd843c5e136c257c28c3b21fc74102da8970406ca5c0c7e406305c5e6e67de5c1708dc1858af07a77a2e05f44159b7103423e8ab32f
…elDB (bugfix)

9bac713 build: make HAVE_O_CLOEXEC available outside LevelDB (bugfix) (Sebastian Falbesoner)
584fd91 init: only use pipe2 if availabile, check in configure (Sebastian Falbesoner)

Pull request description:

  The result of the O_CLOEXEC availability check is currently only set in the Makefile and passed to LevelDB (see `LEVELDB_CPPFLAGS_INT` in `src/Makefile.leveldb.include`), but not defined to be used in our codebase. This means that code within the preprocessor conditional `#if HAVE_O_CLOEXEC` was actually never compiled. On the master branch this is currently used for pipe creation in `src/shutdown.cpp`, PR bitcoin#21007 moves this part to a new module (I found the issue while testing that PR).

  The fix is similar to the one in bitcoin#19803, which solved the same problem for HAVE_FDATASYNC.

  In the course of working on the PR it turned out that pipe2 is not available an all platforms, hence a configure check and a corresponding define HAVE_PIPE2 is introduced and used.

  The PR can be tested by anyone with a system that has pipe2 and O_CLOEXEC available by putting gibberish into the HAVE_O_CLOEXEC block: on master, everything should compile fine, on PR, the compiler should abort with an error. At least that's my naive way of testing preprocessor logic, happy to hear more sophisticated ways :-)

ACKs for top commit:
  laanwj:
    Code review ACK 9bac713

Tree-SHA512: aec89faf6ba52b6f014c610ebef7b725d9e967207d58b42a4a71afc9f1268fcb673ecc85b33a2a3debba8105a304dd7edaba4208c5373fcef2ab83e48a170051
…n.h)

fa59ad5 fuzz: Add missing include (test/util/setup_common.h) (MarcoFalke)

Pull request description:

  `src/test/fuzz/socks5.cpp` is using the symbol `BasicTestingSetup`, which is defined in `src/test/util/setup_common.h`.

  Currently compilation happens to succeed because the needed dependency is indirectly included. Compilation will break as soon as the indirect dependency is broken. According to the dev notes, everything that is used must be included.

  Fix the issue by including the missing include.

ACKs for top commit:
  fanquake:
    ACK fa59ad5

Tree-SHA512: 9359d5d288ebc5a53d753ebed1ee8d49ddcfe12aeb56054ea43654c0d915337bb0dce7c8a7178e94711ff8dacd1b3ea0a2871b21b1709cd9786efc0c1ef532b3
0c471a5 tests: check never active versionbits (Anthony Towns)
3ba9283 tests: more helpful errors for failing versionbits tests (Anthony Towns)

Pull request description:

  Extracted from bitcoin#19573 to make review easier. I also reviewed it myself.

  I added some comments to the test: bitcoin@bae9a45#r585486781

  I also moved some `TestState` changes from the second to the first commit, to reduce the latter diff.

ACKs for top commit:
  ajtowns:
    ACK 0c471a5
  MarcoFalke:
    review ACK 0c471a5 🔓

Tree-SHA512: 61f8d1ecaf38a6cd13db1cf71c89b8c4d2f5852ef77c5e7ecb9bd78eb216545037411641bb101cf0740c5c47845ac327954ee25b676d63779c5f148719ac5caf
e21276a qt test: Don't bind to regtest port (Andrew Chow)

Pull request description:

  The qt tests don't need to bind to the regtest port. By not binding, it will no longer conflict with existing regtest instances and the tests will run as normal.

  Fixes dashpay#10

ACKs for top commit:
  MarcoFalke:
    cr ACK e21276a
  jarolrod:
    re-ACK e21276a, tested on macOS 11.2

Tree-SHA512: 5a269ee043f9aff7900e092c166de71912a2bf86ebe2982b3fb0e26bdebfb91869ee5d0f62082fd608c1288bfb7981f6c8647e504b11176711d7fec993a09164
… flags

5555446 fuzz: Use ConsumeWeakEnum in addrman for service flags (MarcoFalke)

Pull request description:

  This has minimally better performance. Reported by me in bitcoin#20228 (comment)

ACKs for top commit:
  practicalswift:
    Tested ACK 5555446
  vasild:
    ACK 5555446

Tree-SHA512: 4e5f51fe4f2bd7b2f37d0690e41203341ba45c0c9bc9247449cd26cfb5f77dc2ec61df3e4963276f68694e4b3ca3d0a76367a51c4d775501edeb3224d305a261
…ss formatting (RFC 5952)

732c7bd tests: Add test for CNetAddr::ToString IPv6 address formatting (RFC 5952) (practicalswift)

Pull request description:

  Test that `CNetAddr::ToString` formats IPv6 addresses with zero compression and canonicalisation as described in [RFC 5952 ("A Recommendation for IPv6 Address Text Representation")](https://tools.ietf.org/html/rfc5952).

  Solving bitcoin#21466 will hopefully be trivial with the ability to check zero compression correctness against these tests.

ACKs for top commit:
  vasild:
    ACK 732c7bd

Tree-SHA512: 31a1378aa435ba4171490a2e15d7280a175292270eb001b47d367e010c6ac9b83420b82bbeab22211f8f500c69e21878047c87adf216263b3420b6bb2a5d2bfb
…ws and macOS.

7f3a598 qt: Do not use QClipboard::Selection on Windows and macOS. (Hennadii Stepanov)

Pull request description:

  Windows and macOS do [not support](https://doc.qt.io/qt-5/qclipboard.html#notes-for-windows-and-macos-users) the global mouse selection.

  Fixes dashpay#258.

ACKs for top commit:
  promag:
    Code review ACK 7f3a598.
  jarolrod:
    ACK 7f3a598

Tree-SHA512: be2beeef7d25af6f4d4a4548325d8d29f08e4342f499666bc4a670ed468a63195d514077c2cd0dba197e12bd43316fd3e2813cdc0954364b6aa4ae6b90c118bf
f2f2541 remove executable flag for src/net_processing.cpp (Sebastian Falbesoner)

Pull request description:

  The file permissions for `src/net_processing.cpp` have been changed in bitcoin#21713, as discovered by fanquake (bitcoin#21713 (comment)). This PR removes the executable flag again.

ACKs for top commit:
  kiminuo:
    ACK f2f2541 :)
  jnewbery:
    ACK f2f2541
  promag:
    ACK f2f2541.

Tree-SHA512: 1d5a62afb1152029e69fccea2ae53dcb262a91724a5c03dfc4de8c409b280814d0c211c2f9a71f1a6e927f4ed571ba4ac311de9de8ebb797eaf1051674241bdb
b353633 build: mac_alias 2.2.0 (sgulls)

Pull request description:

  Fix make deploy for arm64-darwin

  Accidentally [closed](bitcoin#21555) the PR

ACKs for top commit:
  promag:
    Tested ACK b353633.

Tree-SHA512: 08043792d63894b6738ea93d076cecace1d8b30a623b944170a34492c3838269da87e09878164c760cf321663fb72641a7295070a847ad67d91fc9970ebe5c6a
…onnman::Bind()

36fb036 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack)
4e0d578 test: add net permissions noban/download unit test coverage (Jon Atack)
dde69f2 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack)

Pull request description:

  This is a bugfix follow-up to bitcoin#16248 and bitcoin#19191 that was noticed in bitcoin#21506. Both v0.21 and master are affected.

  Since bitcoin#19191, noban is a multi-flag that implies download, so the conditional in `CConnman::Bind()` using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers.

  The second commit adds unit test coverage to illustrate and test the noban/download relationship and the `NetPermissions` operations involving them.

  The final commit adds documentation and disallows calling `NetPermissions::ClearFlag()` with any second param other than `NetPermissionFlags` "implicit" -- per current usage in the codebase -- because `ClearFlag()` should not be called with any second param that is a subflag of a multiflag, e.g. "relay" or "download," as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this.

ACKs for top commit:
  theStack:
    re-ACK 36fb036 ☕
  vasild:
    ACK 36fb036
  hebasto:
    ACK 36fb036, I have reviewed the code and it looks OK, I agree it can be merged.
  kallewoof:
    Code review ACK 36fb036

Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491
105941b net: use stronger AddLocal() for our I2P address (Vasil Dimov)

Pull request description:

  There are two issues:

  ### 1. Our I2P address not added to local addresses.

  * `externalip=` is used with an IPv4 address (this sets automatically `discover=0`)
  * No `discover=1` is used
  * `i2psam=` is used
  * No `externalip=` is used for our I2P address
  * `listenonion=1 torcontrol=` are used

  In this case `AddLocal(LOCAL_MANUAL)` [is used](https://github.com/bitcoin/bitcoin/blob/94f83534e4b771944af7d9ed0f40746f392eb75e/src/torcontrol.cpp#L354) for our `.onion` address and `AddLocal(LOCAL_BIND)` [for our](https://github.com/bitcoin/bitcoin/blob/94f83534e4b771944af7d9ed0f40746f392eb75e/src/net.cpp#L2247) `.b32.i2p` address, the latter being [ignored](https://github.com/bitcoin/bitcoin/blob/94f83534e4b771944af7d9ed0f40746f392eb75e/src/net.cpp#L232-L233) due to `discover=0`.

  ### 2. Our I2P address removed from local addresses even if specified with `externalip=` on I2P proxy restart.

  * `externalip=` is used with our I2P address (this sets automatically `discover=0`)
  * No `discover=1` is used
  * `i2psam=` is used

  In this case, initially `externalip=` causes our I2P address to be [added](https://github.com/bitcoin/bitcoin/blob/94f83534e4b771944af7d9ed0f40746f392eb75e/src/init.cpp#L1266) with `AddLocal(LOCAL_MANUAL)` which overrides `discover=0` and works as expected. However, if later the I2P proxy is shut down [we do](https://github.com/bitcoin/bitcoin/blob/94f83534e4b771944af7d9ed0f40746f392eb75e/src/net.cpp#L2234) `RemoveLocal()` in order to stop advertising our I2P address (since we have lost I2P connectivity). When the I2P proxy is started and we reconnect to it, restoring the I2P connectivity, [we do](https://github.com/bitcoin/bitcoin/blob/94f83534e4b771944af7d9ed0f40746f392eb75e/src/net.cpp#L2247) `AddLocal(LOCAL_BIND)` which does nothing due to `discover=0`.

  To resolve those two issues, use `AddLocal(LOCAL_MANUAL)` for I2P which is also what we do with Tor.

ACKs for top commit:
  laanwj:
    Code review ACK 105941b

Tree-SHA512: 0c9daf6116b8d9c34ad7e6e9bbff6e8106e94e4394a815d7ae19287aea22a8c7c4e093c8dd8c58a4a1b1412b2575a9b42b8a93672c8d17f11c24508c534506c7
e286cd0 net: flag relevant Sock methods with [[nodiscard]] (Vasil Dimov)

Pull request description:

  Flag relevant Sock methods with `[[nodiscard]]` to avoid issues like the one fixed in bitcoin#21631.

ACKs for top commit:
  practicalswift:
    cr ACK e286cd0: the only changes made are additions of `[[nodiscard]]` and `(void)` where appropriate
  laanwj:
    Code review ACK e286cd0

Tree-SHA512: addc361968d24912bb625b42f4db557791556bf0ffad818252a89a32d76ac22758ec70f8282dcfbfd77eebec20a8e6bb7557c8ed08d50a58de95378c34955973
hebasto and others added 9 commits December 3, 2023 20:44
…ess dialogs are used

4935ac5 qt: Improve GUI responsiveness (Hennadii Stepanov)
7585010 qt, macos: Fix GUIUtil::PolishProgressDialog bug (Hennadii Stepanov)

Pull request description:

  [`QProgressDialog`](https://doc.qt.io/qt-5/qprogressdialog.html) estimates the time the operation will take (based on time for steps), and only shows itself if that estimate is beyond [`minimumDuration`](https://doc.qt.io/qt-5/qprogressdialog.html#minimumDuration-prop).

  The default `minimumDuration` value is [4 seconds](https://doc.qt.io/qt-5/qprogressdialog.html#details), and it could make users think that the GUI is frozen.

  This PR sets `minimumDuration` to zero for all progress dialogs, that affects ones in the `WalletControllerActivity` class.

ACKs for top commit:
  ryanofsky:
    Code review ACK 4935ac5. I'm not very familiar with this API but all the changes and explanations make sense and are very clear, and this seems like it should be an improvement.
  promag:
    Code review ACK 4935ac5.
  jarolrod:
    ACK 4935ac5

Tree-SHA512: 2ddd74e7fd87894d341d2439dbaa544d031a350f7f57d4c7e9fbba977dc24080fe60fd7a80a542b1647f1de9091d7fd04a36eab695088d4d75fb836548e99b5f
…derivation with leading zeros

91ef834 Additional test vector for hardened derivation with leading zeros (Kristaps Kaupe)

Pull request description:

  See [Inconsistent BIP32 Derivations](https://blog.polychainlabs.com/bitcoin,/bip32,/bip39,/kdf/2021/05/17/inconsistent-bip32-derivations.html) and bitcoin/bips#1030.

ACKs for top commit:
  practicalswift:
    cr ACK 91ef834
  sipa:
    ACK 91ef834. Verified that it matches the linked BIP32 update, and that it indeed tests derivation from a private key with leading 0 byte.

Tree-SHA512: 0a3ae7aed15e4e08e9bec5db8de53c6c03ed3b3632f390394eea422597755173cbd2228ff0cfa57f5aae3df9d4cdf03a8ef4725cc8bce86ab7d9c82ab9d479ad
… external warnings are enabled

1111457 build: Disable deprecated-copy warning only when external warnings are enabled (MarcoFalke)

Pull request description:

  Fixes bitcoin#18967

  Alternative to bitcoin#22240

ACKs for top commit:
  fanquake:
    tACK 1111457

Tree-SHA512: 0fc826f26ebbeab662fa7eed2a5cc1630c6c4e612deb91734885fc8bae0352be657ec48ae94ff55a984ac36d27b95cea8d947cc5cf408231d56addecf79db83f
faf1af5 fuzz: Add Temporary debug assert for oss-fuzz issue (MarcoFalke)

Pull request description:

  oss-fuzz is acting weird, so add an earlier assert to help troubleshooting

ACKs for top commit:
  practicalswift:
    cr ACK faf1af5

Tree-SHA512: 85830d7d47cf6b4edfe91a07bd5aa8f7110db0bade8df93868cf276ed04d5dd17e671f769e6a0fb5092012b86aa82bb411fb171411f15746981104ce634c88c1
8f7704d build: improve detection of eBPF support (fanquake)

Pull request description:

  Just checking for the `sys/sdt.h` header isn't enough, as systems like macOS have the header, but it doesn't actually have the `DTRACE_PROBE*` probes, which leads to [compile failures](bitcoin#22006 (comment)). The contents of `sys/sdt.h` in the macOS SDK is:
  ```bash
  #ifndef _SYS_SDT_H
  #define _SYS_SDT_H

  /*
   * This is a wrapper header that wraps the mach visible sdt.h header so that
   * the header file ends up visible where software expects it to be.  We also
   * do the C/C++ symbol wrapping here, since Mach headers are technically C
   * interfaces.
   *
   * Note:  The process of adding USDT probes to code is slightly different
   * than documented in the "Solaris Dynamic Tracing Guide".
   * The DTRACE_PROBE*() macros are not supported on Mac OS X -- instead see
   * "BUILDING CODE CONTAINING USDT PROBES" in the dtrace(1) manpage
   *
   */
  #include <sys/cdefs.h>
  __BEGIN_DECLS
  #include <mach/sdt.h>
  __END_DECLS

  #endif  /* _SYS_SDT_H */
  ```

  The `BUILDING CODE CONTAINING USDT PROBES` section from the dtrace manpage is available [here](https://gist.github.com/fanquake/e56c9866d53b326646d04ab43a8df9e2), and outlines the more involved process of using USDT probes on macOS.

ACKs for top commit:
  jb55:
    utACK 8f7704d
  practicalswift:
    cr ACK 8f7704d
  hebasto:
    ACK 8f7704d, tested on macOS Big Sur 11.4 (20F71) and on Linux Mint 20.1 (x86_64) with depends.

Tree-SHA512: 5f1351d0ac2e655fccb22a5454f415906404fdaa336fd89b54ef49ca50a442c44ab92d063cba3f161cb8ea0679c92ae3cd6cfbbcb19728cac21116247a017df5
…ndex

fafd916 test: Add missing sync_all to feature_coinstatsindex (MarcoFalke)

Pull request description:

  Sync the blocks before invalidating them to ensure all nodes are on the right tip. Otherwise nodes[0] might stay on the "stale" block and the test fails (intermittently)

ACKs for top commit:
  jamesob:
    crACK bitcoin@fafd916

Tree-SHA512: ca567b97b839b56c91d52831eaac18d8c843d376be90c9fd8b49d2eb4a46b801a1d2402996d5dfe2bef3e2c9bd75d19ed443e3f42cc4679c5f20043ba556efc8
7ad414f doc: add comment about CCoinsViewDBCursor constructor (James O'Beirne)
0f8a5a4 move-only(ish): don't expose CCoinsViewDBCursor (James O'Beirne)
615c1ad refactor: wrap CCoinsViewCursor in unique_ptr (James O'Beirne)

Pull request description:

  I tripped over this one for a few hours at the beginning of the week, so I've sort of got a personal vendetta against `CCoinsView::Cursor()` returning a raw pointer.

  Specifically in the case of CCoinsViewDB, if a raw cursor is allocated and not freed, a cryptic leveldb assertion failure occurs on CCoinsViewDB destruction (`Assertion 'dummy_versions_.next_ == &dummy_versions_' failed.`).

  This is a pretty simple change.

  Related to: bitcoin#21766
  See also: google/leveldb#142 (comment)

ACKs for top commit:
  MarcoFalke:
    review ACK 7ad414f 🔎
  jonatack:
    re-ACK 7ad414f modulo suggestion
  ryanofsky:
    Code review ACK 7ad414f. Two new commits look good and thanks for clarifying constructor comment

Tree-SHA512: 6471d03e2de674d84b1ea0d31e25f433d52aa1aa4996f7b4aab1bd02b6bc340b15e64cc8ea07bbefefa3b5da35384ca5400cc230434e787c30931b8574c672f9
e462878 build: remove --enable-determinism configure option (fanquake)

Pull request description:

  This was added by me a while back, with the intention of expanding what this did. That hasn't happened, and this hasn't gained much use. There's also been some discussion of some configure option fatigue, so just remove it for now. Note that `-Wl,--no-insert-timestamp` is also already used in the Guix build.

ACKs for top commit:
  MarcoFalke:
    review ACK e462878
  jarolrod:
    Code Review ACK e462878

Tree-SHA512: ac976f88203eca2a49e296a98693dbe53330e0cb0e273c5ff1fcded30daeb6070cc5beeae35cf9acfdc2279cd64c274d5aeb588aef077aa9bfde39bb23570491
… assert for oss-fuzz issue"

facd567 scripted-diff: Revert "fuzz: Add Temporary debug assert for oss-fuzz issue" (MarcoFalke)

Pull request description:

  No longer needed, as it wouldn't help to debug this issue. See bitcoin#22472 (comment)

ACKs for top commit:
  fanquake:
    ACK facd567

Tree-SHA512: 13352b3529c43d6e65ab127134b32158d3072dc2fbbb326fea9adfeada5a8610d0477ea75748b8b68e7abb3b9869a989df3a3169e92bdd458053d64bae6ed379
@PastaPastaPasta PastaPastaPasta merged commit c516eb0 into dashpay:develop Dec 4, 2023
4 of 5 checks passed
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