💬 MarcoFalke commented on pull request "assumeutxo cleanup: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1286059909)
Same?

💬 MarcoFalke commented on pull request "assumeutxo cleanup: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1286059126)
nit: Missing clang-format on touched line?

💬 MarcoFalke commented on pull request "assumeutxo cleanup: Move IsInitialBlockDownload & NotifyHeaderTip to ChainstateManager":
(https://github.com/bitcoin/bitcoin/pull/28218#discussion_r1286059790)
nit: Add missing `{}` on touched line for multiline if?

💬 1ma commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1668117041)
Thank you for your input @samouraiwallet.

I admit that when I gave my ACK I thought this change was a no-brainer (for the reasons @mikeinspace exposed above) and now I'm conflicted. I wasn't aware of BIP47 v3, and in my personal opinion it is a valid use case. I've reviewed the extent of the work you and others have done on it in the last years and I've realized how extremely pissed you and the more privacy-focused Bitcoin community must be about this proposed change.

However, I have to sa
...

💬 MarcoFalke commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1668122010)
rebased :smiling_face_with_tear:

💬 MarcoFalke commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1668136794)
rebased

🤔 furszy reviewed a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170#pullrequestreview-1565687522)
> I don't think the scenario you describe is likely to happen since we consider ourselves to be in IBD if our tip is older than 24h (which corresponds to ~144 blocks).

Thats not entirely true, misses an important part. We consider in IBD if our tip is older than 24hs (or the custom max tip age) and we haven't completed IBD yet. Once IBD is completed, the node remains out of IBD for the entire software lifecycle.

> > Suppose the node has successfully synced the chain, but later experienced
...

💬 garlonicon commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1668239375)
> Rational: there's lots of ways for people to publish data in the Bitcoin chain, and lots of data has been published.

When it comes to witness data, I can easily prune all of them by downgrading my node to be a non-witness node. How to achieve the same thing for large OP_RETURNs?

> There's no reason for us to place artificial limits on this particular method.

The reason is you will encourage people to downgrade the way they publish data. Witness limit is 4 MB for a reason: it is easy t
...

💬 theStack commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1286143759)
as per developer notes, [assertions shouldn't have side-effects](https://github.com/bitcoin/bitcoin/blob/624333455a5745a7f184d0df531dc348d0ac48dd/doc/developer-notes.md?plain=1#L742), so the following would be preferred:
```suggestion
bool success{chainman.m_blockman.ReadBlockFromDisk(block, pos)};
assert(success);
```
(same for the second test below)

🚀 fanquake merged a pull request: "doc: remove Fedora libdb4-*-devel install docs"
(https://github.com/bitcoin/bitcoin/pull/28231)

💬 RandyMcMillan commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1668341684)
Concept ACK



macOS Catalina Version 10.15.7 (19H2026)

💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1286218964)
Yea, I'm going to change it

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1285999628)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1284653088

> [835f094](https://github.com/bitcoin/bitcoin/commit/835f09452be5449f67fa44ecfa8a178404bff956) It looks like this empty vector might be declarable `constexpr`, at least with clang 16.0.6 arm64. Possibly not uniformly until C++20.

This is a good idea, but I think you are right it requires c++20. With my compiler (clang 13) I see:

```c++
./util/result.h:85:38: error: constexpr variable cannot have non-literal type
...

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1284708030)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1284654294

> [9ec1bda](https://github.com/bitcoin/bitcoin/commit/9ec1bdaff1cba965772a210e1e8326fc4f010aee) Can `Base` here be private instead of protected?

It could, but in general I prefer to use `protected` over `private` when it is safe for extensibility and testing, and in this case I want to support adding a `ResultPtr` type like the one in #26022 which inherits from `Result`. Also, `Base` is just a type alias for a public
...

🤔 ryanofsky reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1563338368)
Updated 9e80d0b754a28733c79a52c8e0431616c31d071c -> 7f883b33bb89205a9d00c2d20d363a36a0167c7c ([`pr/bresult2.39`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.39) -> [`pr/bresult2.40`](https://github.com/ryanofsky/bitcoin/commits/pr/bresult2.40), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/bresult2.39..pr/bresult2.40)) responding to new review comments and also making some internal changes within the PR to reduce unnecessary diffs between commits.

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1286183719)
re: https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1284664601

> ```
> ^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
> ```

Thanks, added

📝 martinus converted_to_draft a pull request: "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile"
(https://github.com/bitcoin/bitcoin/pull/28226)
TLDR:
* Adds a `TRACE_RAII` macro to easily trace runtime of a code block.
* Switch to `CBufferedFile` in `BlockManager::ReadBlockFromDisk` is slightly faster:
* 9% faster unserialization => 1.2% faster `-reindex-chainstate`

---
While profiling `-reindex-changestate` I saw lots of `fread()` calls in in `BlockManager::ReadBlockFromDisk`. This replaces the use of `CAutoFile` with `CBufferedFile` with a small buffer, leading to much fewer calls to
`fread()`, which gives a little speedup.
...

👋 amitiuttarwar's pull request is ready for review: "doc: diversify network outbounds release note"
(https://github.com/bitcoin/bitcoin/pull/28189)

🤔 stickies-v reviewed a pull request: "rpc: Add Arg() default helper"
(https://github.com/bitcoin/bitcoin/pull/28230#pullrequestreview-1565750058)
Approach ACK fa6b2da57ef7ff125a493c7835cb15935255ff8c

This makes for a much more elegant interface throughout the RPC methods, with minimal overhead.

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286137440)
I like how you changed this in your latest force push, can be marked as resolved