💬 virtu commented on pull request "test: various USDT functional test cleanups (27831 follow-ups)":
(https://github.com/bitcoin/bitcoin/pull/27944#issuecomment-1612602693)
Concept ACK. Will take a closer look and test next week.

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1612604508)
The descriptor on which it crashed is `wsh(0)`. This is a bug in the Miniscript logic, exposed by the new check but not introduced in this PR. A `0` script should not be considered sane.

💬 MarcoFalke commented on pull request "util: Safer MakeByteSpan with ByteSpanCast":
(https://github.com/bitcoin/bitcoin/pull/27973#issuecomment-1612623203)
Closing for now to allow for more brainstorming

✅ MarcoFalke closed a pull request: "util: Safer MakeByteSpan with ByteSpanCast"
(https://github.com/bitcoin/bitcoin/pull/27973)

💬 TheCharlatan commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1246307365)
There are many cases where these fatal error codes are eventually ignored as you go up the call stack. I would like to keep this PR focused on the question whether these blockstorage flush functions should at least indicate that they ran into an error. If you want to get a sense of just how many such "ignored" cases there are, I have my own set of patches that achieve something similar to Cory's "bubble up", but with your `Result<T, E>` type and introduced granularly for each fatal error call si
...

💬 willcl-ark commented on pull request "test: Use TestNode datadir_path or chain_path where possible":
(https://github.com/bitcoin/bitcoin/pull/27884#issuecomment-1612636549)
re-ACK aaaa3aefbdfca1c9243057eeefdc19940e60bf18

LGTM with the wallets_path added.

💬 fanquake commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1612637988)
> Yeah, tsan+bdb+aarch64 is impossible to run. You'll have to disable bdb.

Yea, this branch rebased on master + NO_BDB=1 works as expected.

🚀 fanquake merged a pull request: "test: Use TestNode datadir_path or chain_path where possible"
(https://github.com/bitcoin/bitcoin/pull/27884)

💬 glozow commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1612707077)
> Sparrow Wallet displays the fee rate histogram chart in the UI as another "tool in the toolbox". This allows a user to observe trends and adjust the fee rate to place a transaction in a particular fee rate band to suit their time preference.

> a fee rate histogram is a useful tool in the toolbox when estimating fees

I think it's worth noting that a feerate histogram built from individual transaction feerates can be very misleading for fee estimation. A CPFP could look like n low feerate
...

👍 fanquake approved a pull request: "Remove now-unnecessary poll, fcntl includes from net(base).cpp"
(https://github.com/bitcoin/bitcoin/pull/27530#pullrequestreview-1504921462)
ACK 8d9b90a61e3a2a451abfce25328d13aa1e8f749b

🚀 fanquake merged a pull request: "Remove now-unnecessary poll, fcntl includes from net(base).cpp"
(https://github.com/bitcoin/bitcoin/pull/27530)

📝 fanquake opened a pull request: "ci: filter all subtrees from tidy output"
(https://github.com/bitcoin/bitcoin/pull/27996)
We are currently dumping output for some. i.e:
```bash
diff --git a/src/minisketch/src/fields/clmul_1byte.cpp b/src/minisketch/src/fields/clmul_1byte.cpp
index 8826af9..7fd6f2a 100644
--- a/src/minisketch/src/fields/clmul_1byte.cpp
+++ b/src/minisketch/src/fields/clmul_1byte.cpp
@@ -4,21 +4,16 @@
* file LICENSE or http://www.opensource.org/licenses/mit-license.php.*
**********************************************************************/

-/* This file was substantially auto-generat
...

🚀 fanquake merged a pull request: "guix: Clean up manifest"
(https://github.com/bitcoin/bitcoin/pull/27811)

💬 Sjors commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1612735230)
Maybe even more specifically an M2 bug? I've been using macOS for years on two Intel machines, leaving nodes running for weeks or months at a time, and never encountered this.

💬 MarcoFalke commented on pull request "ci: filter all subtrees from tidy output":
(https://github.com/bitcoin/bitcoin/pull/27996#issuecomment-1612774123)
lgtm ACK ce377a0a230d4cefdf03a6c183377fde52cf8f81

💬 MarcoFalke commented on pull request "util: Allow std::byte and char Span serialization":
(https://github.com/bitcoin/bitcoin/pull/27927#discussion_r1246430456)
Just as another follow-up comment to the earlier question if this is too greedy: Yes, it is too greedy, but will actually print a nicer error message compared to using `enable_if` or concepts, because of the general serialization catch-all fallback to the member method.

For example, the following test diff would fail with the general error message for a non-greedy template:

```diff
diff --git a/src/test/serialize_tests.cpp b/src/test/serialize_tests.cpp
index b445ff8ffc..73b9e63011 10064
...

💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1246437951)
Thanks for checking the versions!

> I'm not sure if import asyncio should be added to this file.

`test/lint/lint-python.py` is happy and CI is green, so I guess no need to add it. If I do, then the linter is upset:

```
test/functional/test_framework/test_node.py:7:1: F401 'asyncio' imported but unused
```

👍 hebasto approved a pull request: "ci: filter all subtrees from tidy output"
(https://github.com/bitcoin/bitcoin/pull/27996#pullrequestreview-1505014381)
ACK ce377a0a230d4cefdf03a6c183377fde52cf8f81, I have reviewed the code and it looks OK.

💬 MarcoFalke commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#discussion_r1246447333)
Thanks, may do if I have to re-touch, to not invalidate the previous tests and comments.

💬 MarcoFalke commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#discussion_r1246447882)
Going to leave as-is for now, because the function pre-existing and is only moved, not created in this pull.