💬 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
...

💬 hodlinator commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1816263171)
(Since with the latest non-rebase push you are now touching 2/4 lines mentioning `TEST_DIR_PATH_ELEMENT`, iff you were to re-touch again, it would be nice if you also renamed it to `TESTS_DIR_NAME` as per @ryanofsky's [suggestion](https://github.com/bitcoin/bitcoin/pull/30748#pullrequestreview-2283763614)).

✅ fanquake closed a pull request: "depends: bump miniupnpc to 2.2.8"
(https://github.com/bitcoin/bitcoin/pull/30301)

💬 fanquake commented on pull request "depends: bump miniupnpc to 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2437321903)
I think #31130 is going to go in for `v29.0`. If that doesn't happen, we can reopen here.

💬 rkrux 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_r1816346116)
Was this https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815889280 done in 5c299ecafe6f336cffa145d28036b04b87e26712? I could not find it.

👍 rkrux approved a 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#pullrequestreview-2394740828)
ACK 5c299ecafe6f336cffa145d28036b04b87e26712

Thanks for adding this test, it's easy to follow through.

💬 rkrux 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_r1816349294)








```suggestion
assert_equal(len(node.getorphantxs()), DEFAULT_MAX_ORPHAN_TRANSACTIONS)
```
Storing in another variable is not necessary only for one time usage.

You don't need to invalidate ACKs only for this though.

💬 laanwj commented on pull request "depends: bump miniupnpc to 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2437336876)
> I think https://github.com/bitcoin/bitcoin/pull/31130 is going to go in for v29.0. If that doesn't happen, we can reopen here.

yes... i dont think i've seen any PR that popular in my time here 😄

💬 dergoegge commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2437349340)
Waiting for #31150 to go in