🤔 theStack reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2342669240)
Left some more review comments below; these are mostly pedantic nits that can be done in a follow-up, but also one (minor?) behavior change.

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1786893269)
never-used-nit
```suggestion
```

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1784925614)
```suggestion
for (NodeId nodeid = 0; nodeid < NUM_PEERS; ++nodeid) {
```

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1784927176)
```suggestion
for (NodeId nodeid = 0; nodeid < NUM_PEERS; ++nodeid) {
```

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1786893532)
never-used-nit
```suggestion
```

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1784926619)
```suggestion
Assert(package_to_validate.m_senders.back() < NUM_PEERS);
```

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1784339629)
for this fuzz test it probably doesn't matter if one more output than specified is added, but...
```suggestion
for (size_t o = 0; o < num_outputs; ++o) tx.vout.emplace_back(CENT, P2WSH_OP_TRUE);
```

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1786932357)
nit: that comment is duplicated on the call-site

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1786912728)
nit (these would be moved again in the later commit 4b8cb00fcb429630f717954b5f1c397ce54a8475 ["[refactor] make AlreadyHaveTx and Find1P1CPackage private to TxDownloadImpl"], so might be easier to just fix in a follow-up)
```suggestion
* - m_lazy_recent_rejects
* - m_lazy_recent_rejects_reconsiderable (if include_reconsiderable = true)
* - m_lazy_recent_confirmed_transactions
```

💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1787010637)
Missed this on previous review rounds, but this commit seems to change behaviour, as the <100k memusage condition for txs to be added to the compact-block-extra-txn cache (originally introduced in #9499, commits https://github.com/bitcoin/bitcoin/pull/9499/commits/7f8c8cab1e537809848088d3bfaa13ecca7ac8cc / https://github.com/bitcoin/bitcoin/pull/9499/commits/863edb45b9841c3058e883015c7576e6f69e3cc6) is now also applied to orphan transactions.

📝 AgusR7 opened a pull request: "test: use context managers and add file existence checks in feature_fee_estimation.py"
(https://github.com/bitcoin/bitcoin/pull/31030)
update feature_fee_estimation.py test script to improve resource management and exception safety:

Use context managers for file operations: Replaced all instances of file handling using "open()" with "with open()" statements to ensure files are properly closed after their operations, even if exceptions occur.

Add file existence checks: Before performing file operations like os.utime() and os.remove(), added checks using os.path.exists() to prevent errors when files are missing.

📝 dollarparity opened a pull request: "doc: clarify 'filename' argument in 'loadwallet' RPC"
(https://github.com/bitcoin/bitcoin/pull/31031)
Update the 'loadwallet' RPC help text to specify that the 'filename' should be provided as a path relative to the wallets directory. Add examples showing how to load a wallet from a different directory.

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Ple
...

💬 achow101 commented on pull request "doc: clarify 'filename' argument in 'loadwallet' RPC":
(https://github.com/bitcoin/bitcoin/pull/31031#issuecomment-2392711034)
Duplicate of #30302

⚠️ pilab-gwon opened an issue: "[Testnet] Insufficient data or no feerate found"
(https://github.com/bitcoin/bitcoin/issues/31032)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I encountered an error when attempting to use the estimatesmartfee method.
But node only return `Insufficient data or no feerate found`

Below is the request I sent:
```bash
curl --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "estimatesmartfee", "params": [1] }' -H 'content-type: text/plain;' http://localhost:80
```


### Expected behaviour

Expected output:
```
{"re
...

👍 Sjors approved a pull request: "qt6, test: Handle deprecated code"
(https://github.com/bitcoin-core/gui/pull/839#pullrequestreview-2347416833)
tACK 5625840c11db2065a1c8d8de3babb6466eda04d4

I only tested on Intel macOS 15.0 with Homebrew qt@5. Checked that the warnings still appear with `build/src/qt/test/test_bitcoin-qt`.

💬 Sjors commented on pull request "qt6, test: Handle deprecated code":
(https://github.com/bitcoin-core/gui/pull/839#discussion_r1787335639)
5625840c11db2065a1c8d8de3babb6466eda04d4: note to other reviewers: https://doc.qt.io/qt-6/qtest.html#QVERIFY_THROWS_EXCEPTION

👍 itornaza approved a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2347440969)
reACK 98c1536852d1de9a978b11046e7414e79ed40b46

💬 promag commented on pull request "qt6: Fix linking when configured with `-DENABLE_WALLET=OFF`":
(https://github.com/bitcoin-core/gui/pull/837#issuecomment-2393139603)
ACK 5be34bacf6d07fb73a0cedfb63a384968d538f3e, all other changes in 33657e1c958146312e4c68765a92871920401396 are not required, makes the code harder to read.

💬 hebasto commented on pull request "qt6: Fix linking when configured with `-DENABLE_WALLET=OFF`":
(https://github.com/bitcoin-core/gui/pull/837#discussion_r1787391324)
> Reworked.

Reverted back.

> How the `RPCExecutor::request` method compiles when this line doesn't exist?

Due to https://github.com/bitcoin-core/gui/blob/5be34bacf6d07fb73a0cedfb63a384968d538f3e/src/qt/rpcconsole.h#L24

🚀 hebasto merged a pull request: "qt6, test: Handle deprecated code"
(https://github.com/bitcoin-core/gui/pull/839)