💬 TheCharlatan commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730402391)
nit: `static constexpr`

💬 TheCharlatan commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730408130)
I think this should be de-globalized and renamed like I've done here: https://github.com/TheCharlatan/bitcoin/commit/a2c048322c67436e02a3a9607400f65f10dd6564

The `avaialble_fds` global variable there are also kind of silly, since it is only there to be logged at a later point in time, but I don't feel strongly if it should go or not either way.

💬 TheCharlatan commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1730402947)
Why did you not put `MAX_ADDNODE_CONNECTIONS` in here?

💬 pablomartin4btc commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#issuecomment-2308962681)
Updates:
- Addressed @hebasto's feedback.
- Fixed the [error message window inconsistency](https://github.com/bitcoin-core/gui/pull/762#issuecomment-2276259183) detected by @hebasto [above](https://github.com/bitcoin-core/gui/pull/762#pullrequestreview-2219281894).

<details>
<summary>Now running <code>qt</code> from the command line with <code>bitcoin-qt.exe -signet -about</code> (or similar command with an invalid argument) will show the correct chain type icon at the top left of the tit
...

📝 theStack opened a pull request: "test: fix `TestShell` initialization (late follow-up for #30463)"
(https://github.com/bitcoin/bitcoin/pull/30714)
Creating a `TestShell` instance as stated in the [docs](https://github.com/bitcoin/bitcoin/blob/master/test/functional/test-shell.md) currently fails on master:
```
$ python3
Python 3.10.13 (main, Mar 15 2024, 07:36:23) [Clang 16.0.6 ] on openbsd7
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.path.insert(0, "/home/thestack/bitcoin/test/functional")
>>> from test_framework.test_shell import TestShell
>>> test = TestShell().setup(num_nodes=2,
...

👍 tdb3 approved a pull request: "http: set TCP_NODELAY when creating HTTP server"
(https://github.com/bitcoin/bitcoin/pull/30675#pullrequestreview-2259359677)
ACK 03d49d0f25ab5660524d5ddd171de677a808b984

Nice speedup! Saw a 28x speedup for a full block.


Ran the test on a full block (`ab -k -c 1 -n 100 http://localhost:8332/rest/block/00000000000000000000b2ffb308ce769ad1c1b239c78553c4e3f931a74d5569.bin`).

27.1 release:
```
Requests per second: 5.08 [#/sec] (mean)
Time per request: 196.684 [ms] (mean)
```

master (6d546336e800f7b8990fececab6bc08413f28690):

```
Requests per second: 5.28 [#/sec] (mean)
Time per request:
...

💬 theStack commented on pull request "test: fix `TestShell` initialization (late follow-up for #30463)":
(https://github.com/bitcoin/bitcoin/pull/30714#issuecomment-2308964530)
cc @hebasto, maybe you have a better idea how to solve this; the current approach works but feels a bit hacky :)

💬 fjahr commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2308980712)
Thanks for addressing my feedback @ryanofsky ! I hope we can merge #29553 soon and then I will get back to this with another full review. But it does look very good to me already.

> I'm especially interested if you have ideas on splitting up documentation and code changes into separate PRs.

I'm unsure if it's really needed because I find clean refactoring commits like those here can be reviewed quicker than other changes, so I would be fine to keep it as is just based on volume of the chan
...

🤔 i-am-yuvi reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2259369009)
tACK

Tested and Built successfully on:

- MacOS 14.6.1 without GUI and Wallet
- Ubuntu 24.04 without GUI and Wallet

Also ran tests with `ctest`.

💬 jamesob commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2309028680)
Concept ACK. This'd be useful, especially in conjunction with https://github.com/bitcoin/bitcoin/pull/30708.

💬 furszy commented on pull request "wallet: Write best block to disk before backup":
(https://github.com/bitcoin/bitcoin/pull/30678#discussion_r1730380590)
The idea of moving this to `Flush()` sounded promising to me too but I don't think we can/should do it after some code walking. At least not without doing some extra restructuring.

The shutdown procedure stops the wallet, closing the db connection, before the wallet is destructed (which is when the flushing occurs). So any db write during `Flush` should cause a segfault due to the db handler access.

Also, moving this to `Flush()` would write the same locator twice during shutdown. And this
...

💬 furszy commented on pull request "Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings":
(https://github.com/bitcoin/bitcoin/pull/30697#issuecomment-2309041765)
Take this one: https://github.com/furszy/bitcoin-core/commit/b017949d83c1c47e6aea708ac89f0d81cead1064.
Have removed the `OptionModel` changes because those require further changes to the `Node` interface and implementation.

💬 maflcko commented on pull request "test: XORed blocks test follow up":
(https://github.com/bitcoin/bitcoin/pull/30669#issuecomment-2309536360)
ACK e1d5dd732d5dc641faf1dde316275c84b6bb224b

💬 TheCharlatan commented on pull request "validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime":
(https://github.com/bitcoin/bitcoin/pull/27427#issuecomment-2309536891)
Concept ACK, but needs rebase and the title and description should be updated, since this is no longer replacing the height, but is instead adding logic to the start time of a period.

📝 hebasto opened a pull request: "qt: 28.0 translations update"
(https://github.com/bitcoin/bitcoin/pull/30715)
The 28.x branching off is [scheduled](https://github.com/bitcoin/bitcoin/issues/29891) for today, so it's [time](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-branch-off) to fetch the recent translations from [Transifex.com](https://www.transifex.com/bitcoin/bitcoin) using the [`bitcoin-maintainer-tools/update-translations.py`](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/update-translations.py) tool.

A similar PR from the previous release
...

💬 hebasto commented on pull request "qt: 28.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/30715#issuecomment-2309568245)
cc @stickies-v @pablomartin4btc @jarolrod

💬 Sjors commented on pull request "Have miner account for timewarp mitigation, activate on regtest, lower nPowTargetTimespan to 144 and add test":
(https://github.com/bitcoin/bitcoin/pull/30681#issuecomment-2309596835)
@maaku this would be better to discuss on the mailinglist and/or Delving.

There's ongoing discussion what addition / other mitigation is needed to handle another variant of the timewarp that is currently not fixed: https://delvingbitcoin.org/t/zawy-s-alternating-timestamp-attack/1062

Maybe that can be combined with what you have in mind.

We can always launch a testnet5 with completely different rules.

👍 theStack approved a pull request: "test: XORed blocks test follow up"
(https://github.com/bitcoin/bitcoin/pull/30669#pullrequestreview-2260068418)
re-ACK e1d5dd732d5dc641faf1dde316275c84b6bb224b

🤔 stratospher reviewed a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2228592166)
Concept ACK. nice to have more consistent behaviour for what to expect with the negated version of the config flags compared to on master.

💬 stratospher commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1730774338)
d079370: (feel free to ignore/not a blocker since `noconnect=0` isn't recommended)

would `IsArgSet` be simpler compared to `GetArgs()/IsArgNegated()` here? or is something else meant by preferring `GetArgs()/IsArgNegated()` in most cases (but not all) over `IsArgSet`.


i just found this behaviour of `noconnect` confusing - even though both do the same thing, `GetArgs()` returns empty in one case and not empty in the other case. (and similarly for `IsArgNegated`)

- when `noconnect=1`

...