🤔 mzumsande reviewed a pull request: "net: don't lock cs_main while reading blocks in net processing"
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-2043866221)
Code Review ACK 75d27fefc7a04ebdda7be5724a014b6a896e7325

💬 mzumsande commented on pull request "net: don't lock cs_main while reading blocks in net processing":
(https://github.com/bitcoin/bitcoin/pull/26326#discussion_r1592887228)
nit: maybe mentioning the block hash and/or height in the unconditional logs here and below could be helpful so in case of a disk failure, users could reproduce the failure easier (e.g. with getblock RPC).

💬 theStack commented on pull request "net: Replace ifname check with IFF_LOOPBACK in Discover":
(https://github.com/bitcoin/bitcoin/pull/29984#issuecomment-2099051316)
post-merge ACK a68fed111be393ddbbcd7451f78bc63601253ee0

Interestingly, libpcap still implements both the "brittle" and the `IFF_LOOPBACK` flag way to detect loopback devices (https://github.com/the-tcpdump-group/libpcap/blob/0797ed7340f93618fd097ad870dfc1dde7cecc4f/pcap.c#L1042-L1053), but the code was seemingly introduced in 1998 (https://github.com/the-tcpdump-group/libpcap/blob/0797ed7340f93618fd097ad870dfc1dde7cecc4f/CHANGES#L1443-L1445) and I suspect that these days you won't find any Un
...

💬 kristapsk commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2099071507)
Concept ACK on using different software for various DNS seeders. Need to do more review / testing on this one.

💬 willcl-ark 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-2099073663)
I ran some benchmarks of IBD to block 800,000 vs master for comparison, and got some similar, if slightly less impressive, results with default dbcache.

With `-dbcache=16384`:

- master@ fdb41e08: 9607 s
- master@ fdb41e08 + 7f15e71f7e762645dbd1ea5eba9ecc6f9ad60236: 9351 s
- 3% faster with this change

With `-dbcache=450`:

- master@ fdb41e08: 15338 s
- master@ fdb41e08 + 7f15e71f7e762645dbd1ea5eba9ecc6f9ad60236: 13246 s
- ~16% faster with this change

I only did a single run of e
...

💬 kristapsk commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1592914219)
I was expecting this comment from you. :)

Named it same way as existing startup / config option (`-datacarriersize`), but I'm open to other names, as in fact it only limits `OP_RETURN`. What name seems to be good from Knots perspective? It would be good to not unnecessary break compatibility between Core and Knots.

💬 willcl-ark commented on issue "Memory leak with `rest/block` REST endpoint and `getblock` RPC when verbosity >=2":
(https://github.com/bitcoin/bitcoin/issues/30052#issuecomment-2099084632)
I could not reproduce an OOM, although I did observe resource usage increasing a little before eventually plateauing.

I measured resident set size using python's `psutil` module, whilst making 2500 REST requests for random blocks in the blockchain between block 0 and 842460. The node was in `blocksonly` mode, and had no peer connections.


I haven't investigated any deeper than this y
...

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#issuecomment-2099086073)
With 194e84accced947ef63c6db389bc62a2b58cffa3, since reindexing regenerates undo data, and undo data shouldn't be added until all existing blocks are, it seems like there is no reason for the `AddToBlockFileInfo` function to worry about resetting the `BlockfileCursor::undo_file` field or even accessing the block storage cursors at all. So I think the following simplification would make sense:

```diff
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -941,22 +941,11 @@ bool
...

✅ willcl-ark closed an issue: "(EDITED) How was the average size of blk*.data chosen?"
(https://github.com/bitcoin/bitcoin/issues/30056)

💬 willcl-ark commented on issue "(EDITED) How was the average size of blk*.data chosen?":
(https://github.com/bitcoin/bitcoin/issues/30056#issuecomment-2099093796)
This issue tracker is used to track technical issues related to the Bitcoin Core code base.

General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com/) or the #bitcoin IRC channel on the [Libera Chat](https://libera.chat/) network.

For proposed protocol changes you can post to the bitcoin-dev [mailing list](https://lists.linuxfoundation.org/mailman/listinfo/bitcoin-dev).

For general bitcoin discussion you can try
...

👍 kristapsk approved a pull request: "refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition"
(https://github.com/bitcoin/bitcoin/pull/29086#pullrequestreview-2043937365)
cr utACK cc67d33fdac45357b593b1faff3d1735e5fe91ba

💬 andrewtoth 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-2099135435)
FWIW re: #29662 I did not notice any difference in compaction time at startup on an SSD. It takes about 5 seconds to finish with `debug=leveldb` both on master and this branch.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#issuecomment-2099144448)
Concept ACK

Built and ran the unit tests.

The new messages seem to be the opposite of what the tests do, tough.

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592963190)
OK, the much simpler change I pushed in b6be80ca8c74852d4e2c1476527c4300be2125b8 has successfully [failed](https://cirrus-ci.com/task/6592292239179776) CI.

I am still a bit worried there may be other (like me) who have a `.venv` dir in their source dir, which will always fail the check. But got both options available now anyway.

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592988891)
Not sure about re-implementing `which`. Why not just call `mlc --version` or `mlc --help`, like in the shellcheck check?

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592994881)
Is the conversion to string objects needed? According to the docs, `join` will make a String by itself, no? https://doc.rust-lang.org/std/slice/trait.Join.html#associatedtype.Output-1

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2099213678)
Rebased due to the conflict with merged bitcoin/bitcoin#29494.

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1593008473)
My thought was to avoid running another subprocess which will then make the same syscalls (excluding checking it's executable).

`--version` is less code on our side though, and more robust, so I will switch to that.

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1593009480)
Yes, fixed in ffc691ac70b4a652fdf62e3a28876a5feb8d97c8

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1593012869)
Some of the option fields do currently hold references and it might be a good idea to move ownership of those referred values to the kernel Context eventually. I agree though that the comment should reflect the current code, not some future hypothetical.