:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/30056)

💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2099665991)
https://github.com/bitcoin/bitcoin/pull/29868/checks?check_run_id=24699627882:
```bash
In file included from common/run_command.cpp:13:
./util/subprocess.h: In member function ‘int subprocess::Popen::wait()’:
./util/subprocess.h:1062:7: error: unused variable ‘ret’ [-Werror=unused-variable]
1062 | int ret = WaitForSingleObject(process_handle_, INFINITE);
| ^~~
```

🚀 fanquake merged a pull request: "doc: fix broken relative md links"
(https://github.com/bitcoin/bitcoin/pull/30025)

👍 fanquake approved a pull request: "ci: Exclude feature_init for now in valgrind task"
(https://github.com/bitcoin/bitcoin/pull/30054#pullrequestreview-2044582434)
ACK fab179d10243e85cdb172a9f08bcb7ec19ddf74d

✅ fanquake closed an issue: "test: Intermittent issue in feature_init.py", line 88, in run_test with node.wait_for_debug_log([terminate_line]): AssertionError: [node 0] Expected messages "[b'scheduler thread start']" does not partially match log:"
(https://github.com/bitcoin/bitcoin/issues/30011)

🚀 fanquake merged a pull request: "ci: Exclude feature_init for now in valgrind task"
(https://github.com/bitcoin/bitcoin/pull/30054)

📝 luke-jr opened a pull request: "Add option dbfilesize to control LevelDB target ("max") file size"
(https://github.com/bitcoin/bitcoin/pull/30059)
Debug option to control LevelDB file sizes. Since LevelDB seems to always overshoot, "max" didn't seem fitting.

Intended to be followed up with a change to the default in a rebased #30039

💬 fanquake commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#issuecomment-2099704210)
> So I'd say to close this and bump to 16 wholesale?

I think generally, that would be more straightforward, then supporting varying compiler + std lib combinations, and seems possible to do at this point.

💬 fanquake commented on pull request "guix: build with glibc 2.31":
(https://github.com/bitcoin/bitcoin/pull/29987#issuecomment-2099704721)
> Is there a benefit to this? Just dropping patches?

No, it's not just dropping patches. It's about us not having to maintain an EOL branch of glibc, us getting bugfixes (if relevant) to the branch we are using, us getting closer to properly supporting hardening features, fully static builds etc, by using a glibc that supports them.

💬 maflcko commented on pull request "test: added test coverage to loadtxoutset could not open file":
(https://github.com/bitcoin/bitcoin/pull/30053#discussion_r1593521401)
With 5 ACKs this nit does not seem worth it to address at this point. Can do in one of the other pulls that touch this file, in a follow-up?

👍 S3RK approved a pull request: "Fix waste calculation in SelectionResult"
(https://github.com/bitcoin/bitcoin/pull/28366#pullrequestreview-2044816148)
Code review ACK e95b9159380f2de7f9a6e7a202cc171ad285ee6c

I'm not sure whether 44db79b0c48d6b1a26dba6ab01c2a296d610c06b is useful since it's fully replaced by the following commit.

Also added a bunch of nits in the tests about comments and potential simplifications, but nothing blocking.

💬 S3RK commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1593503078)
This comment needs to be adjusted

💬 S3RK commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1593507686)
nit: this could be pulled to the begging of the test next to other declarations and then reused in the tests above (and below as well)

💬 S3RK commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1593510238)
nit: same variable is already defined within another local scope above, could be reused. See another comment.

💬 S3RK commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1593513968)
nit: do you want to add assert for the comment above? It's not obvious that the statement holds given variable declarations are hundreds of lines above.

the comment:
"Negative waste when the long term fee is greater than the current fee and change_cost < - (inputs * (fee - long_term_fee))"

💬 S3RK commented on pull request "Fix waste calculation in SelectionResult":
(https://github.com/bitcoin/bitcoin/pull/28366#discussion_r1593517046)
nit: could use `exact_target - excess` if you introduce `exact_target` var

💬 S3RK commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#discussion_r1593544047)
In this case `random.sample` shuffles the order of participants, but I don't see how it matters since all participants are equivalent anyway

💬 S3RK commented on pull request "tests: improve wallet multisig descriptor test and docs":
(https://github.com/bitcoin/bitcoin/pull/29154#issuecomment-2099956492)
The change looks good to me modulo addressing comments from achow101.

💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2099974737)
> [...] use a recent block hash as a provably unspendable public key (instead of all zeros).
>
> Well, we now have a genesis block with the hash in the message and the all zeros public key. I don't think that makes a difference anymore?

I find it more elegant, but only if we need another genesis block.

> In practice today that probably means pre-mining a handful of blocks to add the test cases and then checkpointing the end of the pre-mine.

That makes sense if we want the test cases
...

💬 laanwj commented on pull request "net: Replace ifname check with IFF_LOOPBACK in Discover":
(https://github.com/bitcoin/bitcoin/pull/29984#issuecomment-2100006946)
Thanks for checking! it's kind of a belt-and-suspenders check: we only look for routable addresses in Discover, so unless someone assigned a routable address to the loopback interface-which would be really strange-it doesn't actually influence the result.