💬 luke-jr commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2099010611)
Why?
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2099010611)
Why?
👍 ryanofsky approved a pull request: "blockstorage: Separate reindexing from saving new blocks"
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2041313158)
Code review ACK 194e84accced947ef63c6db389bc62a2b58cffa3. I left a lot of comments, but everything looks right here and the code is a lot nicer than before.
(https://github.com/bitcoin/bitcoin/pull/29975#pullrequestreview-2041313158)
Code review ACK 194e84accced947ef63c6db389bc62a2b58cffa3. I left a lot of comments, but everything looks right here and the code is a lot nicer than before.
💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591338361)
In commit "validation, blockstorage: Separate code paths for reindex and saving new blocks" (a17eacab1f8790afc5f89ba2ee3e34da4c9369e1)
It looks like previously there would have been an error here if `dbp->IsNull()` was true, and now there will not be an error. This is probably a good change, since AcceptBlock should not be looking at block positions, just passing them on.
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591338361)
In commit "validation, blockstorage: Separate code paths for reindex and saving new blocks" (a17eacab1f8790afc5f89ba2ee3e34da4c9369e1)
It looks like previously there would have been an error here if `dbp->IsNull()` was true, and now there will not be an error. This is probably a good change, since AcceptBlock should not be looking at block positions, just passing them on.
💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591463595)
In commit "blockstorage: Add Assume for fKnown / snapshot chainstate" (d6b0bb6dd0d26f3e10386d1afbafd8d52a12b2c5)
This comment seems really disconnected from the statement below it, because the statement does not mention reindexing or the snapshot chainstate at all. I left a suggestion to improve the comment below.
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591463595)
In commit "blockstorage: Add Assume for fKnown / snapshot chainstate" (d6b0bb6dd0d26f3e10386d1afbafd8d52a12b2c5)
This comment seems really disconnected from the statement below it, because the statement does not mention reindexing or the snapshot chainstate at all. I left a suggestion to improve the comment below.
💬 ryanofsky commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591477885)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)
Would be helpful to clarify with meaning of `true` with `/*fFinalize*/=true`
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1591477885)
In commit "blockstorage: split up FindBlockPos function" (a2ae0a33c7c30678721d7e7d37d8e6170b013383)
Would be helpful to clarify with meaning of `true` with `/*fFinalize*/=true`
💬 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
...
(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
...
(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
...
(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(...)`)
(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.
(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/
(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
(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)
(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?
(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
(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).
(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
...
(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.
(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
...
(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.
(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.