💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591461829)
In commit "blockstorage: Add Assume for fKnown / snapshot chainstate" (d6b0bb6dd0d26f3e10386d1afbafd8d52a12b2c5)

I don't think changing this `if` statement is good.

Adding the Assume call above seems good, since it provides information about the context this code is called in and could potentially catch bugs if the code is run in an unanticipated state.

But It's less clear what benefit there is to adding the `!fKnown &&` condition to this if statement. It just makes the logic more comp
...

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1592739006)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)

I think this description is technically accurate, but I got confused by it and thought it was wrong because "the 8 byte serialization header" sounds like something that is part of `CBlock` serialization, when actually it is referring to separator fields written by `WriteBlockToDisk` *before* the serialized `CBlock`.

Would suggest changing comment to "pos: the position of the serialized CBloc
...

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1592707738)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)

Two suggestions, maybe for later commits or a followup PR:

Now that `pos` is an output parameter instead of being an in/out parameter, it would be better to just drop it entirely and make `FindBlockPos` return `FlatFilePos` like `SaveBlockToDisk`, instead of returning `bool`. This would make it more obvious what the function inputs and outputs are, and also make sure the output value is cons
...

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1592833124)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)

I think `SaveBlockPos` would be a less ambiguous name for this function than `AddToBlockFileInfo`. It would also be consistent with `FindBlockPos` and I think make the `AcceptBlock` code more obvious (`if (dbp) SaveBlockPos(...) else SaveBlockToDisk(...)`)

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1592824870)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)

I think this comment is a little confusing, because it isn't obvious that `position_known` can only be true during reindexing. Could potentially clarify this, though not necessary since this code will be deleted in the next commit.

💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1592848815)
In commit "validation, blockstorage: Separate code paths for reindex and saving new blocks" (a17eacab1f8790afc5f89ba2ee3e34da4c9369e1)

s/do/to/

💬 achow101 commented on pull request "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29494#issuecomment-2099011321)
ACK fa09451f8e6799682d7e7c863f25334fd1c7dce3

🚀 achow101 merged a pull request: "build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes"
(https://github.com/bitcoin/bitcoin/pull/29494)

💬 luke-jr commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2099035706)
@achow101 Are you sure you want to put this on a common domain with other things?

🤔 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