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: bitcoin#17934, #21338, #21390, #21445, #21602, #21606, #21609, #21676, bitcoin-core/gui#260, #5976

Merged
merged 11 commits into from
Apr 12, 2024

Conversation

knst
Copy link
Collaborator

@knst knst commented Apr 9, 2024

Issue being fixed or feature implemented

Regular backports from bitcoin v22

Note for reviewers:

PRs bitcoin#17934 and bitcoin#21606 have been backported partially in past.

What was done?

Removed unused sanitizer rules (see bitcoin#21366 and #5055)

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

@knst knst added this to the 21 milestone Apr 9, 2024
@knst
Copy link
Collaborator Author

knst commented Apr 9, 2024

@knst knst force-pushed the bp-v22-p6 branch from f94a58d to aca303f
Rebased on the top of develop

CI before rebase succeeded: https://gitlab.com/dashpay/dash/-/pipelines/1245926027

@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Apr 9, 2024
UdjinM6
UdjinM6 previously approved these changes Apr 9, 2024
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.

one potential conflict, LGTM otherwise

utACK

test/lint/extended-lint-cppcheck.sh Show resolved Hide resolved
@UdjinM6 UdjinM6 dismissed their stale review April 10, 2024 19:20

needs rebase to fix ff-only merge check

MarcoFalke and others added 11 commits April 11, 2024 02:26
…in wallet_ groups

c62f9bc test: use fewer blocks in wallet_groups and move sync call (Jon Atack)
3a16b5e test: add missing logging to wallet_groups.py (Jon Atack)

Pull request description:

  - add logging (particularly useful as the tests are somewhat slow)
  - generate 101 blocks instead of 110
  - move `sync_all` call into the loop, so fewer blocks are synced on each call, to hopefully see fewer CI timeouts as in https://bitcoinbuilds.org/index.php?ansilog=88eee99e-1727-44ed-b778-3b9c75c33928.log

  ```
  L2742     File "/home/ubuntu/src/test/functional/wallet_groups.py", line 162, in run_test
  L2743       self.sync_all()
  test_framework.authproxy.JSONRPCException: 'syncwithvalidationinterfacequeue' RPC took longer than 960.000000 seconds. Consider using larger timeout for calls that take longer to return. (-344)
  ```

ACKs for top commit:
  MarcoFalke:
    cr ACK c62f9bc

Tree-SHA512: 711deafcd589cb8196cb207ff882e0f2ab6b70828a6abad91f83f535974cc430a56b9e8a960fd233d31d610932a0d48b49ee681aae564d145a3040288ecda8f8
581791c test: add functional test for anchors.dat (bruno)

Pull request description:

  This PR adds a functional test for anchors.dat.

  It creates a node and adds 2 outbound block-relay-only connections and 5 inbound connections.
  When the node is down, anchors.dat should contain the 2 addresses from the outbound block-relay-only connections.

ACKs for top commit:
  MarcoFalke:
    Concept ACK 581791c
  hebasto:
    ACK 581791c

Tree-SHA512: 77038b09e36ee5ae473a26d6f566c0ed283af258c34df8486706a24f72b05abab621a293ac886d03849bc45bc28be7336137252225b25aff393baa6b5238688c
4f2653a test: Use deterministic chain in utxo set hash test (Fabian Jahr)
4973c51 test: Remove wallet dependency of utxo set hash test (Fabian Jahr)
1a27af1 rpc: Improve gettxoutsetinfo help (Fabian Jahr)

Pull request description:

  Follow-ups to bitcoin#19145:
  - Small improvement on the help text of RPC gettxoutsetinfo
  - Using deterministic blockchain in the test `functional/feature_utxo_set_hash.py`
  - Removing wallet dependency in the test `functional/feature_utxo_set_hash.py`

  Split out of bitcoin#19521.

ACKs for top commit:
  MarcoFalke:
    review ACK 4f2653a 👲

Tree-SHA512: 92927b3aa22b6324eb4fc9d346755313dec44d973aa69a0ebf80a8569b5f3a7cf3539721ebdba183737534b9e29b3e33f412515890f0d0b819878032a3bba8f9
This PR is follow-up for dash#5055 and based on bitcoin#21366 which is DNM
…ase memory limit

de3ae78 ci: increase CPU count of sanitizer job to increase memory limit (fanquake)

Pull request description:

  According to the [docs](https://cirrus-ci.org/guide/linux/#linux-containers):
  > For each CPU you can't get more than 4G of memory.

  thus if we want this job to have 24GB of memory, we need to increase the CPU count to 6.

  It's currently [failing with](https://github.com/bitcoin/bitcoin/runs/2273962280):
  >  Requested memory is too high! You can request at most 4G per CPU

Top commit has no ACKs.

Tree-SHA512: 0a4da5649d061425190a373859274c78ca5587cd2d6e27905ec548f124ed114a0133215cb2eff22ffc182f50c3a53df58e7c9832b44db6e37d7ea59ec96a4775
fa21239 cirrus: Use SSD cluster for speedup (MarcoFalke)

Pull request description:

  Haven't tested, but this might be faster: https://twitter.com/fedor/status/1354505744293502980

ACKs for top commit:
  fanquake:
    ACK fa21239 - going to merge this now given there is a speedup, and it's enough to fix the fuzz task the is continually timing out.

Tree-SHA512: b24161ba2ed16fcf23ac6098100b04f7f6eb8936bb312a35fcd8e632568b877bfc09a1e522836fe298a2cd87a53a91e3b501a7f2e1c81cc0edb57c59aecffb0d
faaf395 fuzz: Extend psbt fuzz target a bit (MarcoFalke)

Pull request description:

  Previously it only merged the psbt with itself, now it tries to merge another.

ACKs for top commit:
  practicalswift:
    Tested ACK faaf395

Tree-SHA512: e1b1d31a47d35e1767285bc2fda176c79cb0550d6d383fe467104272e61e1c83f6cbc0c7d6bbc0c3027729eec13ae1f289f8950117ee91e0fb3703e66d5e6918
…x option

223b1ba doc: Use CONFIG_SITE instead of --prefix (Hennadii Stepanov)

Pull request description:

  The current examples of `--prefix=...` option usage to point `configure` script to appropriate `depends` directory is not [standard](https://www.gnu.org/prep/standards/html_node/Directory-Variables.html). This causes some [confusion](bitcoin#16691) and a bit of inconvenience.

  Consider a CentOS 7 32 bit system. Packages `libdb4-devel`, `libdb4-cxx-devel`, `miniupnpc-devel` and `zeromq-devel` are unavailable from repos. After recommended build with depends:
  ```
  cd depends
  make
  cd ..
  ./autogen.sh
  ./configure --prefix=$PWD/depends/i686-pc-linux-gnu
  make
  ```
  a user is unable to `make install` compiled binaries neither locally (to `~/.local`) nor system-wide (to `/usr/local`) as `--prefix` is set already.

  Meanwhile, the standard approach with using [`config.site`](https://www.gnu.org/software/automake/manual/html_node/config_002esite.html) files allows both possibilities:

  ```
  cd depends
  make
  cd ..
  ./autogen.sh
  CONFIG_SITE=$PWD/depends/i686-pc-linux-gnu/share/config.site ./configure --prefix ~/.local
  make
  make install
  ```

  or

  ```
  CONFIG_SITE=$PWD/depends/i686-pc-linux-gnu/share/config.site ./configure
  make
  sudo make install  # install to /usr/local
  ```

  Moreover, this approach is used in [Gitian descriptors](https://github.com/bitcoin/bitcoin/tree/master/contrib/gitian-descriptors) already.

ACKs for top commit:
  practicalswift:
    ACK 223b1ba: patch looks correct
  fanquake:
    ACK 223b1ba

Tree-SHA512: 46d97924f0fc7e95ee4566737cf7c2ae805ca500e5c49af9aa99ecc3acede4b00329bc727a110aa1b62618dfbf5d1ca2234e736f16fbdf96d6ece5f821712f54
b8e5d0d qt: Handle exceptions in SendCoinsDialog::sendButtonClicked slot (Hennadii Stepanov)
1ac2bc7 qt: Handle exceptions in TransactionView::bumpFee slot (Hennadii Stepanov)
bc00e13 qt: Handle exceptions in WalletModel::pollBalanceChanged slot (Hennadii Stepanov)
eb6156b qt: Handle exceptions in BitcoinGUI::addWallet slot (Hennadii Stepanov)
f7e260a qt: Add GUIUtil::ExceptionSafeConnect function (Hennadii Stepanov)
64a8755 qt: Add BitcoinApplication::handleNonFatalException function (Hennadii Stepanov)
af7e365 qt: Make PACKAGE_BUGREPORT link clickable (Hennadii Stepanov)

Pull request description:

  This PR is an alternative to bitcoin#18897, and is based on Russ' [idea](bitcoin#18897 (review)):
  > IMO it would be nice to have a followup PR that eliminated the one-line forwarding methods ...

  Related issues
  - dashpay#91
  - bitcoin#18643

  Qt docs: https://doc.qt.io/qt-5.12/exceptionsafety.html#exceptions-in-client-code

  With this PR the GUI handles the wallet-related exception, and:
  - display it to a user:

  ![Screenshot from 2021-04-01 02-55-59](https://user-images.githubusercontent.com/32963518/113226183-33ff8480-9298-11eb-8fe6-2168834ab09a.png)

  - prints a message to `stderr`:
  ```

  ************************
  EXCEPTION: 18NonFatalCheckError
  wallet/wallet.cpp:2677 (IsCurrentForAntiFeeSniping)
  Internal bug detected: '!chain.findBlock(block_hash, FoundBlock().time(block_time))'
  You may report this issue here: https://github.com/bitcoin/bitcoin/issues

  bitcoin in QPushButton->SendCoinsDialog

  ```

  - writes a message to the `debug.log`
  - and, if the exception is a non-fatal error, leaves the main window running.

ACKs for top commit:
  laanwj:
    Code review ACK b8e5d0d
  ryanofsky:
    Code review ACK b8e5d0d. This is great! I think more improvements are possible but implementation is very clean and I love how targeted each commit is. Changes since last review: adding more explanatory text, making links clickable, reorganizing.

Tree-SHA512: a9f2a2ee8e64b993b0dbc454edcbc39c68c8852abb5dc1feb58f601c0e0e8014dca81c72733aa3fb07b619c6f49b823ed20c7d79cc92088a3abe040ed2149727
d3b0b08 doc: release notes for new listbanned fields (Jarol Rodriguez)
60290d3 test: increase listbanned unit test coverage (Jon Atack)
3e978d1 rpc: add time_remaining field to listbanned (Jarol Rodriguez)
5456b34 rpc: add ban_duration field to listbanned (Jarol Rodriguez)
c95c616 doc: improve listbanned help (Jarol Rodriguez)
dd3c8ea rpc: swap position of banned_until and ban_created fields (Jarol Rodriguez)

Pull request description:

  This PR adds a `ban_duration` and `time_remaining` field to the `listbanned` RPC command. Thanks to jonatack, this PR also expands the `listbanned` test coverage to include these new fields

  It's useful to keep track of `ban_duration` as this is another data point on which to sort banned peers. I found this helpful in adding additional context columns to the GUI `bantablemodel` as part of a follow-up PR. As [suggested](bitcoin#21602 (comment)) by jonatack, `time_remaining` is another useful user-centric data point.

  Since a ban always expires after its created, the `ban_created` field is now placed before the `banned_until` field. This new ordering is more logical.

  This PR also improves the `help listbanned` output by providing additional context to the descriptions of the `address`, `ban_created`, and `banned_until` fields.

  **Master: listbanned**
  ```
  [
    {
      "address": "1.2.3.4/32",
      "banned_until": 1617691101,
      "ban_created": 1617604701
    },
    {
      "address": "135.181.41.129/32",
      "banned_until": 1649140716,
      "ban_created": 1617604716
    }
  ]
  ```

  **PR: listbanned**
  ```
  [
    {
      "address": "1.2.3.4/32",
      "ban_created": 1617775773,
      "banned_until": 1617862173,
      "ban_duration": 86400,
      "time_remaining": 86392
    },
    {
      "address": "3.114.211.172/32",
      "ban_created": 1617753165,
      "banned_until": 1618357965,
      "ban_duration": 604800,
      "time_remaining": 582184
    }
  ]
  ```

ACKs for top commit:
  jonatack:
    re-ACK d3b0b08
  hebasto:
    ACK d3b0b08, tested on Linux Mint 20.1 (x86_64).
  MarcoFalke:
    review ACK d3b0b08 🕙

Tree-SHA512: 5b83ed2483344e546d57e43adc8a1ed7a1fff292124b14c86ca3a1aa2aec8b0f7198212fabff2c5145e7f726ca04ae567fe667b141254c7519df290cf63774e5
… in rpc_tests

fa40d6a test: Reset mocktime in the common setup (MarcoFalke)
fa78590 test: Use mocktime to avoid intermittent failure (MarcoFalke)

Pull request description:

  See bitcoin#21602 (comment)

ACKs for top commit:
  jonatack:
    Code review ACK fa40d6a
  jarolrod:
    ACK fa40d6a

Tree-SHA512: 4967e006f3d2c4eb92f03c9086a6abe3190ad54755d251c30d20422c574bb1a154c06f3d5bcb0d4deaa3c4abfd3864d743b71d84897edd358e829bb42233ad12
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.

re-utACK

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 21ad71c

@PastaPastaPasta PastaPastaPasta merged commit 7aa8f54 into dashpay:develop Apr 12, 2024
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RPC Some notable changes to RPC params/behaviour/descriptions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants