💬 laanwj commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#discussion_r1590651112)
+1 on a constant, but i don't think it's approprioate to reuse `MAX_BLOCKFILE_SIZE`, better to define a new one

💬 laanwj commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2095395201)
> This branch is 27% (!) faster than master

That's impressive!

> From what I understand, this patch reduces the frequency of flushes

Not only the frequency of flushes; another potential advantage here is that leveldb will spend less time open()ing and close()ing files to maintain its allowed number of open files (eg the `fd_limiter` stuff).

💬 TheCharlatan commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1590664391)
I think it is better off eventually in the kernel library eventually: https://github.com/bitcoin/bitcoin/pull/28690/files#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77R735

👍 hebasto approved a pull request: "kernel: Remove key module from kernel library"
(https://github.com/bitcoin/bitcoin/pull/29252#pullrequestreview-2040262030)
ACK a885a166cec6d84d08600f12b25d912bd28af80e.

💬 hebasto commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1590699911)
nit: Add `#include <key.h>`?

💬 maflcko commented on pull request "Prevent file descriptor exhaustion from too many RPC calls":
(https://github.com/bitcoin/bitcoin/pull/27731#issuecomment-2095457721)
> removing libevent

Nice. Is there a tracking issue?

👍 TheCharlatan approved a pull request: "rpc: return warnings as an array instead of just a single one"
(https://github.com/bitcoin/bitcoin/pull/29845#pullrequestreview-2040301855)
ACK 42fb5311b19582361409d65c6fddeadbee14bb97

👍 TheCharlatan approved a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2040303535)
Re-ACK 5eeafeb302dcf56dd5c16c1e4785a41a132344c7

💬 fanquake commented on pull request "build, bench, msvc: Add missing benchmarks":
(https://github.com/bitcoin/bitcoin/pull/29773#issuecomment-2095507661)
Is the intention to try and merge this as-is, or are the Windows issues going to be investigated and fixed (can draft if so)?

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2095510288)
Re https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2083201174

> But that is not the case. The libbitcoin_util still depends on libbitcoin_crypto, libbitcoin_consensus and libbitcoin_common libraries.
> If such an outcome is expected, maybe update the doc/design/libraries.md accordingly?

I understand this PR more as providing clarity of what should be in the util library and #28690 as actually realizing the diagram.

📝 josibake opened a pull request: "Compare `COutPoints` lexicographically"
(https://github.com/bitcoin/bitcoin/pull/30046)
Broken out from #28122

---

BIP352 requires the smallest output lexicographically, which is my primary motivation for adding this as the default way of compare `COutPoint`s (e.g. https://github.com/bitcoin/bitcoin/blob/5b06ccf1dc63f56dbc35368a37b9a72af671f15f/src/common/bip352.cpp#L149)

Outside of needing this for #28122 , I think it makes more sense to compare `COutPoints` by their serialization for any use case that needs ordering since the serialization is what appears in the final t
...

👋 fanquake's pull request is ready for review: "[27.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/29888)

💬 TheCharlatan commented on pull request "depends: swap some cctools tools for LLVM tools":
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2095519873)
Guix build (aarch64):
```
6a215012d268b68b25d3120c63f7154169061d71bef8e3a434dc44d6e2c8bf06 guix-build-4313febae66b/output/aarch64-linux-gnu/SHA256SUMS.part
7469b754ce7632c175116ffb1490771b9a4b635c33ccf711fd5df2cf08c6d6d4 guix-build-4313febae66b/output/aarch64-linux-gnu/bitcoin-4313febae66b-aarch64-linux-gnu-debug.tar.gz
7ffd30c4e1894555a1172e13106d8fbf6291f357073fb17840e653f98fe3599f guix-build-4313febae66b/output/aarch64-linux-gnu/bitcoin-4313febae66b-aarch64-linux-gnu.tar.gz
f8d0a35036
...

💬 hebasto commented on pull request "build, bench, msvc: Add missing benchmarks":
(https://github.com/bitcoin/bitcoin/pull/29773#issuecomment-2095522393)
> Is the intention to try and merge this as-is, or are the Windows issues going to be investigated and fixed (can draft if so)?

The former. The issue is tracked in https://github.com/bitcoin/bitcoin/issues/29816.

💬 darosior commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#discussion_r1590747954)
I fail to see how it's not functionally equivalent?

👍 maflcko approved a pull request: "rpc: return warnings as an array instead of just a single one"
(https://github.com/bitcoin/bitcoin/pull/29845#pullrequestreview-2040332641)
ACK 42fb5311b19582361409d65c6fddeadbee14bb97 🔺

<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+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 42fb5311b19582361409d65c6f
...

💬 maflcko commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1590743718)
```suggestion
can temporarily be restored by running with configuration
```

nit (only if you re-touch): Could remove the executable name, as it can be `bitcoin-qt` as well, or a different name.

💬 maflcko commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1590753252)
> kernel

I guess it is a bit odd to have `g_timeoffset_warning` in the kernel, but happy to review either approach.

📝 josibake opened a pull request: "refactor: Model the bech32 charlimit as an Enum"
(https://github.com/bitcoin/bitcoin/pull/30047)
Broken out from #28122

---

Bech32(m) was defined with a 90 character limit so that certain guarantees for error detection could be made for segwit addresses (see https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#checksum-design).

However, there is nothing about the encoding scheme itself that requires a limit of 90 and in practice bech32(m) is being used without the 90 char limit (e.g. lightning invoices, silent payments). Further, increasing the character limit doesn't d
...

💬 josibake commented on pull request "Compare `COutPoints` lexicographically":
(https://github.com/bitcoin/bitcoin/pull/30046#discussion_r1590763287)
See https://github.com/bitcoin/bitcoin/blob/246e2a24c5460504c799fbc6312ef075f0ca4e72/src/test/data/bip352_send_and_receive_vectors.json#L383 for an example of how the old method (a.hash < b.hash, a.vout < b.vout) will yield a different ordering than comparing outpoints lexicographically.