💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815888974)
Thanks! Updated in [5c299ec](https://github.com/bitcoin/bitcoin/pull/31040/commits/5c299ecafe6f336cffa145d28036b04b87e26712)

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815889024)
Thanks! Updated in [5c299ec](https://github.com/bitcoin/bitcoin/pull/31040/commits/5c299ecafe6f336cffa145d28036b04b87e26712)

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815889280)
Thanks! Updated in [5c299ec](https://github.com/bitcoin/bitcoin/pull/31040/commits/5c299ecafe6f336cffa145d28036b04b87e26712)

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2436659077)
> This looks like a separate issue. I was talking about orphan resolution removing the tx, you are talking about TxRequestTracker expiration removing it.

Yes, see my comments above. I got it there are current issues in the orphan resolution removing the transaction with a `ForgetTxHash` before we received it again, and wrongly see it as unsolicited.

> I wasn't changing a random line of code: the added code is guarded by a future P2P version upgrade and therefore unreachable and untestable
...

💬 ariard commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2436662747)
if one can go and publish on the mailing list in which core release this is expected to land and indicates the expected deployment timelines for the downstream softwares. learning a bit from the ecosystem controversies raised when `mempoolfullrbf` was actually introduced back in 24.0.1.

💬 pythcoiner commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2436768523)
concept ACK

💬 maflcko commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2437086690)
rfm?

🤔 hebasto reviewed a pull request: "doc: replace `-?` with `-h` and `-help`"
(https://github.com/bitcoin/bitcoin/pull/31118#pullrequestreview-2394439589)
Post-merge ACK 33a28e252a7349c0aa284005aee97873b965fcfe.

💬 i-am-yuvi commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437131445)
Concept ACK!!

🚀 fanquake merged a pull request: "fuzz: wallet: add target for `CreateTransaction`"
(https://github.com/bitcoin/bitcoin/pull/29936)

👍 maflcko approved a pull request: "Drop miniupnp dependency"
(https://github.com/bitcoin/bitcoin/pull/31130#pullrequestreview-2394558598)
Almost lgtm. Didn't review the gui changes and the rebase was borked.

ACK 2a541e93cf13362e52c62e4d01d34a602843471e 📏

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU
...

💬 maflcko commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1816223837)
3c0fe09cc2fac18175aac35a54d64c9f6cd4482b: This merge conflict was solved incorrectly. Please be careful when rebasing. See also diff3, which should make rebasing easier, if used correctly: https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md#rebasingmerging-code

💬 laanwj commented on pull request "rpc, cli: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31135#issuecomment-2437200247)
maybe... im not sure it's a super good first issue, implementing it properly isn't super complicated but does require some knowledge of how various parts work and interact

💬 garlonicon commented on pull request "miner: Reorg Testnet4 minimum difficulty blocks":
(https://github.com/bitcoin/bitcoin/pull/31117#issuecomment-2437215836)
> where a network with realistic mining dynamics is important

What about using "getblocktemplate" on mainnet, and mining blocks, which will quickly become stale? If you want to have worthless coins, then they should be stale, in other cases, they will be traded.

Another possibility is to use "Pay to Proof of Work" addresses on signet/regtest. Then, moving a coin will require grinding a DER signature, to get it below N bytes, so it will require some mining power.

> just pointing out that
...

💬 maflcko commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816298165)
nit: Maybe to avoid the churn here (changing the same line of code twice), and the c_str bloat, a simple alias could be introduced in an early commit, so that only the alias has to be changed internally in later commits?

For example

```cpp
template<typename... Args>
void fuzz_fmt(const std::string &fmt, const Args&... args){(void)tfm::format_raw(fmt.c_str(), args...);}

💬 rkrux commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1816314261)
Agree, having this assertion would be good because the valid tx would still not make to mempool in this case.

🤔 rkrux reviewed a pull request: "test: added test to assert TX decode rpc error on submitpackage rpc"
(https://github.com/bitcoin/bitcoin/pull/31139#pullrequestreview-2394696857)
LGTM, will ACK again once @instagibbs's suggestion is also added.

https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1815824499

💬 rkrux commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1816314768)
Are these blank lines intentional?

💬 laanwj commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437290310)
i've checked that no references no miniupnp as dependency remain after this change (except in historical release notes ofc).

Also, successful guix build of 2a541e93cf13:
```
d0674bc3aa12577fb8d0319d51b34ab8650b810f56868a701b388f4f91cfc9b2 guix-build-2a541e93cf13/output/aarch64-linux-gnu/SHA256SUMS.part
6be1f0a8fa6596bd4e6e68996b9a53cb438fb727e55755f6bf101656e1517c47 guix-build-2a541e93cf13/output/aarch64-linux-gnu/bitcoin-2a541e93cf13-aarch64-linux-gnu-debug.tar.gz
a9386bc64b0222ce03a3a
...

👍 hodlinator approved a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2394616462)
re-ACK 80d35bc20797e59571e82fe6919705b33dcc40cd

* Commit message improvements.
* `test_path` -> `test_name` (improved naming).
* Made `test_name` be included as part of randomized test directory when *not* setting `-testdatadir`. Including it is helpful for locating test data of a specific test when multiple tests fail. :+1:

Verified correct unit test directory behavior using `build/src/test/test_bitcoin -- -testdatadir=/tmp/foo/`, then not passing the `-testdatadir` and simultaneously r
...