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 06 07 #6050

Merged
12 commits merged into from
Jun 10, 2024
Merged

backport: trivial 2024 06 07 #6050

12 commits merged into from
Jun 10, 2024

Conversation

PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

Batch of trivial backports

What was done?

Trivial backports

How Has This Been Tested?

Built; ran tests locally

Breaking Changes

None

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

  • 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 and others added 5 commits June 7, 2024 09:45
@fanquake @PastaPastaPasta
Merge bitcoin#25231: ci: Install documented packages for "Win64" CI task
b750896 
3ffc190 ci: Install documented packages for "Win64" CI task (Hennadii Stepanov)

Pull request description:

  This minor change has the following benefits:
  - it follows the [documented](https://github.com/bitcoin/bitcoin/blob/master/doc/build-windows.md#building-for-64-bit-windows) way for modern Ubuntu distros (this CI task uses jammy)
  - it makes package installation time shorter as no need to install the `g++-mingw-w64-x86-64-win32` package
  - (not directly related to this repo) in https://github.com/bitcoin-core/gui-qml Qt 5.15.3 (but not 5.15.2) build system goes [wild](https://cirrus-ci.com/task/6231535933194240) otherwise

ACKs for top commit:
  fanquake:
    ACK 3ffc190
  jarolrod:
    ACK 3ffc190

Tree-SHA512: 41fd6deedb3febc90cc4f2037dfbf840d82ef5b1dd950a0ff458cae6c1b1024559b21c8e1135c2d37780e80dd3f9f9751d638120443d0d60c22ac160cf693e2a
@PastaPastaPasta
Merge bitcoin#25333: test: Fix out-of-range port collisions
1fae0c2 
fa7a711 test: Fix out-of-range port collisions (MacroFake)

Pull request description:

  Otherwise the test will fail if two tests running in parallel use the same port. See bitcoin#25096 (comment) and bitcoin#25312

ACKs for top commit:
  dergoegge:
    ACK fa7a711 - This gets rid of some rather arbitrary choices for ports in some of our functional tests that can cause port collisions across test runs, resulting in intermittent failures.

Tree-SHA512: ac73da8a498230b992ab12e1ee3c4ff3d868cd63c00d2c71537d156cb7c8f8be8598ec574646b17c5a44ae3ac5bb54bf29d300f054a36cec6f6ce8054a0da0a4
@fanquake @PastaPastaPasta
Merge bitcoin#25738: depends: use a patch instead of sed in libxcb
73d64c4 
8f1ff48 libxcb: use a patch instead of sed (fanquake)

Pull request description:

  To remove the unneeded pthread-stubs requirement.

  Should almost be enough to close bitcoin#16838.

  seds dead (mostly). The usage left in `qt.mk` are for substituting runtime values.

ACKs for top commit:
  hebasto:
    ACK 8f1ff48.

Tree-SHA512: 2b6ebbe98a838d8e08e54737292b02176ff4c85a541ae1ec0c590c75e33ba92289628b88ca3144f2e214f4327515f7fd22c39687312f44183b759815c092b24f
@achow101 @PastaPastaPasta
Merge bitcoin#25925: doc: add {import,list}descriptors to list of d…
12b438c 
…escriptor RPCs

6242314 doc: add `{import,list}descriptors` to list of descriptor RPCs (Sebastian Falbesoner)

Pull request description:

  This PR adds the missing RPCs `importlistdescriptors` ([since v0.21](https://github.com/bitcoin/bitcoin/blob/1420547ec30a24fc82ba3ae5ac18374e8e5af5e5/doc/release-notes/release-notes-0.21.0.md?plain=1#L405)) and `listdescriptors` ([since v22](https://github.com/bitcoin/bitcoin/blob/1420547ec30a24fc82ba3ae5ac18374e8e5af5e5/doc/release-notes/release-notes-22.0.md?plain=1#L175)) to the list of RPCs supporting descriptors in descriptors.md. Also changes the description of `importmulti` slightly to point out that it only works for legacy wallets.

ACKs for top commit:
  S3RK:
    ACK 6242314
  achow101:
    ACK 6242314
  aureleoules:
    ACK 6242314.
  brunoerg:
    ACK 6242314

Tree-SHA512: e8905c800b0c9a760e3380efebe2fb015c321a891dd4bf283039486d9d3b382b2c76901fcc8413acf435ed9832f76d9828efd70ba5ce62d4be65e87672bbd0a2
@achow101 @PastaPastaPasta
Merge bitcoin#25896: wallet: Log when Wallet::SetMinVersion sets a di…
c72ef29 
…fferent minversion

835bd27 Wallet::SetMinVersion - Log the new minversion (Ali Sherief)

Pull request description:

  This change prints a single additional line in the debug.log when bitcoin-cli loads a wallet using `loadwallet` (*not* `createwallet`).

  When Bitcoin Core creates a wallet, it's `minversion` is set to `FEATURE_BASE`, which is 10500. However, once the wallet is unloaded using `unloadwallet` or through program termination, and subsequently loaded again, `loadwallet` updates the `minversion` in the wallet.dat file to `FEATURE_LATEST`, currently 169900.

  The current logging format prints the very old wallet version during `createwallet`, and then the actual version in calls to `loadwallet`. This has confused at least one person ([reference](https://bitcointalk.org/index.php?topic=5410650.0) - I was the one who asked there if there were plans to change that behavior, and was subsequently redirected here by achow), so it will be very helpful to users to explicitly specify in the logs what the walletdb is doing.

ACKs for top commit:
  achow101:
    ACK 835bd27

Tree-SHA512: 967c8c617e06a84915ddb147378ec3c8b0343e45f43145ec78df9cbc0201867f49c8e11cd068c403eb5ec06e07d38c3c0d3864dad8edc5efbb134a3fb30be41f
@PastaPastaPasta PastaPastaPasta added this to the 21 milestone Jun 9, 2024
UdjinM6
UdjinM6 requested changes Jun 10, 2024
View reviewed changes
doc/descriptors.md Outdated Show resolved Hide resolved
doc/bips.md Outdated Show resolved Hide resolved
test/functional/mempool_compatibility.py Outdated Show resolved Hide resolved
doc/release-notes-26628.md Outdated Show resolved Hide resolved
src/wallet/rpcdump.cpp Outdated Show resolved Hide resolved
achow101 and others added 5 commits June 10, 2024 11:00
@achow101 @PastaPastaPasta
Merge bitcoin#24269: test: add functional test for -discover
3db3bd0 
bff05bd test: add functional test for -discover (brunoerg)

Pull request description:

  This PR adds a functional test for `-discover`. It tests different scenarios where `localaddresses` should be empty or may contain the addresses. Obs: `localaddresses` is not always accurate, so it's not possible to ensure (100%) it will contain any addresses.

  https://github.com/bitcoin/bitcoin/blob/515200298b555845696a07ae2bc0a84a5dd02ae4/src/init.cpp#L449

  Obs: See bitcoin#24258  - It adds test coverage for this field but for nodes with proxy.

ACKs for top commit:
  mzumsande:
    Code review ACK bff05bd
  achow101:
    ACK bff05bd
  rajarshimaitra:
    tACK bff05bd

Tree-SHA512: 8782497c146bce1ba86fda6146f3847465d7069f2cb6b84f2afc8f3b43efa813442bffe7447e9ce02adee304100b60365409bf0e5d875dfb880038442feec2a6
@PastaPastaPasta
Merge bitcoin#26380: Revert "test: check importing wallets when block…
245df94 
…s are pruned throw an error"

e1eadaa Revert "test: check importing wallets when blocks are pruned throw an error" (Aurèle Oulès)

Pull request description:

  The test doesn't pass (not detected by the normal CI, because it is an extended test):

  ```
  Traceback (most recent call last):
    File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/test_framework.py", line 133, in main
      self.run_test()
    File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/feature_pruning.py", line 480, in run_test
      self.wallet_test()
    File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/feature_pruning.py", line 361, in wallet_test
      assert_raises_rpc_error(-4, "Importing wallets is disabled when blocks are pruned", self.nodes[2].importwallet, "abc")
    File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 130, in assert_raises_rpc_error
      assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
    File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 145, in try_rpc
      raise AssertionError(
  AssertionError: Expected substring not found in error message:
  substring: 'Importing wallets is disabled when blocks are pruned'
  error message: 'Only legacy wallets are supported by this command'.
  ```

  So revert it for now, which will be done anyway in https://github.com/bitcoin/bitcoin/pull/24865/commits. (This commit is taken from there)

ACKs for top commit:
  andrewtoth:
    ACK e1eadaa

Tree-SHA512: 10f556ea1aa97dc9cb64f91055977eecb9534f658170aabb4909c3e85ca6c20588c7a021219356fab678e0e2bec999d347facd00054f07a9445ad393e6353b4c
@fanquake @PastaPastaPasta
Merge bitcoin#26546: test: remove unused class NodePongAdd1
69b19cb 
40bdc8a test: remove unused class `NodePongAdd1` (Sebastian Falbesoner)

Pull request description:

  This class was introduced in commit fa33654 ("net: Use mockable time for ping/pong, add tests"), but actually never used.

ACKs for top commit:
  stickies-v:
    ACK 40bdc8a

Tree-SHA512: b5a6552e4f2e0b7e368a071cc53b9a8e6f5d1950565a9fda8eb1971a01d8be0541d066842723ef44174fe8189925fa36f2defb6d7bf8d104abc77de410cc4c13
@PastaPastaPasta
Merge bitcoin#26100: doc: clarify that NetPermissionFlags::Implicit i…
478fe51 
…s only about whitelists

f362920 doc: clarify that NetPermissionFlags::Implicit is only about whitelists (Vasil Dimov)

Pull request description:

  `NetPermissionFlags::Implicit` applies just to connections from `-whitebind` or `-whitelist`, clarify that in its comment.

ACKs for top commit:
  Zero-1729:
    crACK f362920
  aureleoules:
    ACK f362920
  hernanmarino:
    re ACK f362920

Tree-SHA512: 03f6f8be221c6819bdd0b5b56b69b4e3a6dd25e5ca5a247eeb1261113144b9b74cf064a0b7815317782a0a18365dd3dab97963bd238e9b231dbe7e1cf0395683
@PastaPastaPasta
Merge bitcoin#26666: refactor: Deleted unreachable code in httpserver…
7c28b01 
….cpp

8f5c560 refactor: Refactored RequestMethodString function to follow developer notes (JoaoAJMatos)
7fd3b94 refactor: Deleted unreachable code in httpserver.cpp (JoaoAJMatos)

Pull request description:

  Some of the code in httpserver.cpp was unreachable, and didn't follow the developer notes.
  Continuation of [bitcoin#26570 ](bitcoin#26570)

ACKs for top commit:
  stickies-v:
    re-ACK [8f5c560](bitcoin@8f5c560)

Tree-SHA512: ba8cf4c6dde9e2bb0ca9d63a0de86dfa37b070803dde71ac8384c261045835697a2335652cf5894511b3af8fd99f30e1cbda4e4234815b8b39538ade90fab3f9
@PastaPastaPasta PastaPastaPasta force-pushed the develop-trivial-2024-06-07 branch from fbdf3e0 to 9b336f2 Compare June 10, 2024 16:02
UdjinM6
UdjinM6 requested changes Jun 10, 2024
View reviewed changes
doc/release-notes-6050.md Outdated Show resolved Hide resolved
MarcoFalke and others added 2 commits June 10, 2024 11:26
@PastaPastaPasta
Merge bitcoin#26628: RPC: Reject RPC requests with same named paramet…
6e1a8c1 
…er specified multiple times

8c3ff7d test: Suggested cleanups for rpc_namedparams test (Ryan Ofsky)
d1ca563 bitcoin-cli: Make it an error to specify the "args" parameter two different ways (Ryan Ofsky)
6bd1d20 rpc: Make it an error server-side to specify same named parameter multiple times (Ryan Ofsky)
e2c3b18 test: Add RPC tests for same named parameter specified more than once (Ryan Ofsky)

Pull request description:

  Make the JSON-RPC server reject requests with the same named parameter specified multiple times, instead of silently overwriting earlier parameter values with later ones.

  Generally JSON keys are supposed to unique, and their order isn't supposed to be significant, so having the server silently discard duplicate keys is error-prone. Most likely if an RPC client is sending a request with duplicate keys it means something is wrong with the request and there should be an error.

  After this change, named parameters are still allowed to specified multiple times on the `bitcoin-cli` command line, since `bitcoin-cli` automatically replaces earlier values with later values before sending the JSON-RPC request. This makes sense, since it's not unusual for the order of command line options to be significant or for later command line options to override earlier ones.

ACKs for top commit:
  MarcoFalke:
    review ACK 8c3ff7d 🗂
  kristapsk:
    ACK 8c3ff7d
  stickies-v:
    ACK 8c3ff7d

Tree-SHA512: 2d1357dcc2c171da287aeefc7b333ba4e67babfb64fc14d7fa0940256e18010a2a65054f3bf7fa1571b144d2de8b82d53076111b5f97ba29320cfe84b6ed986f
@hebasto @PastaPastaPasta
Merge bitcoin-core/gui#682: Don't directly delete abandoned txes from…
6777ab7 
… GUI

e75d227 Minor fix: Don't directly delete abandoned txes (John Moffett)

Pull request description:

  This fully closes bitcoin#12179. Currently, when a user abandons a transaction by clicking "Abandon Transaction" in the context menu, a call is made to remove it from the GUI view:

  `model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);`

  (The `false` parameter is for `bool showTransaction`)

  This behavior is probably unwanted, as the transaction is not actually removed from the wallet and would show up again if the node is restarted.

  However, the previous line, `model->wallet().abandonTransaction(hash);`, changes the underlying model and calls `NotifyTransactionChanged(wtx.GetHash(), CT_UPDATED);`, which queues a signal that eventually calls back to `updateTransaction`, this time with `showTransaction` set to `true`. This runs on a separate thread, so it gets called *after* the 'subsequent' `updateTransaction`. The transaction gets removed from the GUI and immediately added back.

  In a nutshell, `updateTransaction` gets called twice. The first (direct) call deletes the transaction from the GUI. The second (sent via a queued signal) brings it back to the GUI. The first direct call is redundant and unwanted. Worse, if the `abandonTransaction` call fails for any reason, the transaction still gets removed from the GUI. (This is what caused bitcoin#12179. It can still be triggered if, eg., a user clicks "Abandon Transaction" the moment after a new block is found.)

  There are no conditions (to my knowledge) where an abandoned transaction should be directly removed from the GUI. If the underlying model changes, the deletion should be reflected anyway by the queued signal to `updateTransaction`.

  The behavior is borne out by the QT logs. To reproduce, send a transaction with RBF enabled, then bump the fee, then 'abandon transaction' on the first transaction. The logs will show something like this:

  ```
  2022-11-28T14:48:00Z [qt] GUI: "NotifyTransactionChanged: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 status= 1"
  2022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
  2022-11-28T14:48:00Z [qt] GUI: "    inModel=1 Index=381-382 showTransaction=0 derivedStatus=2"
  2022-11-28T14:48:00Z [qt] GUI: "TransactionTablePriv::updateWallet: 2c5811484f1adec92a739a5e70b453b03eaed0f7cc0538fbd0ee1589e586b951 1"
  2022-11-28T14:48:00Z [qt] GUI: "    inModel=0 Index=381-381 showTransaction=1 derivedStatus=0"
  ```

  Notice the duplicate `updateWallet` calls with different `showTransaction` values.

ACKs for top commit:
  hebasto:
    ACK e75d227
  jarolrod:
    tACK e75d227

Tree-SHA512: 00f150f747c2ee1605af861a21d5c3b9773a4a9985e8dab62e48bd32885b1bfa4e8cbf805ad61af77aec9d3ccefaed3f4311a29086aa8c22d55d5326ba68ece6
@PastaPastaPasta PastaPastaPasta force-pushed the develop-trivial-2024-06-07 branch from 33903a4 to 6777ab7 Compare June 10, 2024 16:26
UdjinM6
UdjinM6 approved these changes Jun 10, 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 6777ab7

@PastaPastaPasta PastaPastaPasta closed this pull request by merging all changes into dashpay:develop in 0254959 Jun 10, 2024
@PastaPastaPasta PastaPastaPasta deleted the develop-trivial-2024-06-07 branch June 10, 2024 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@UdjinM6 UdjinM6 Awaiting requested review from UdjinM6

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

Successfully merging this pull request may close these issues.
5 participants
@PastaPastaPasta @UdjinM6 @fanquake @achow101 @hebasto