💬 alfonsoromanz commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2211804813)
Post-merge question: Shouldn't this TODO be deleted because of the new test introduced in this PR?

> - TODO: Valid snapshot file, but referencing a snapshot block that turns out to be
> invalid, or has an invalid parent

The `test_snapshot_block_invalidated` function seems to address this scenario by invalidating the snapshot base block and one of its parents, ensuring the snapshot cannot be loaded if the base block or its parent is invalid.

✅ achow101 closed an issue: "Illegal Instruction in `CoinStatsIndex::CustomAppend`"
(https://github.com/bitcoin/bitcoin/issues/30402)

💬 achow101 commented on issue "Illegal Instruction in `CoinStatsIndex::CustomAppend`":
(https://github.com/bitcoin/bitcoin/issues/30402#issuecomment-2211805935)
> At what height did it crash? If the height was 112516, this is probably a duplicate of #26362.

Ah, indeed.

💬 achow101 commented on issue "signed overflow in coinstats index":
(https://github.com/bitcoin/bitcoin/issues/26362#issuecomment-2211806088)
Just ran into this.

📝 luke-jr opened a pull request: "GUI/OptionsDialog: Prefer to stretch actual options area rather than waste space"
(https://github.com/bitcoin-core/gui/pull/827)

💬 fjahr commented on issue "signed overflow in coinstats index":
(https://github.com/bitcoin/bitcoin/issues/26362#issuecomment-2211806760)
I will reopen my fix, I just got lost on the the upgrade mechanics for indices and then lost focus.

📝 fjahr opened a pull request: "test: Remove already resolved assumeutxo todo comment"
(https://github.com/bitcoin/bitcoin/pull/30403)
The case in the comment seems to be sufficiently addressed by #30267 but I forgot to remove the Todo. Thanks to alfonsoromanz for noticing this.

💬 fjahr commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#issuecomment-2211808920)
> Post-merge question: Shouldn't this TODO be deleted because of the new test introduced in this PR?

Duh, you are right, fixed with #30403. Thanks!

💬 luke-jr commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2211809968)
Seems more like kernel shouldn't have policy stuff at all, but that's out of scope for this PR. Added your commit

💬 alfonsoromanz commented on pull request "test: Remove already resolved assumeutxo todo comment":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2211817861)
ACK 9d5a89509031ce924b9685a6e2e4b19cfef50a9e

I wonder if this is a good place to discuss other (potentially addressed) TODOs?

For example, interesting starting states could be loading a snapshot when the current chain tip is:

`- TODO: An ancestor of snapshot block`: isn't this already addressed when successfully loading the snapshot into node1 [here](https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_assumeutxo.py#L310)? Node1's chain tip is at `START_HEIGTH` (199) b
...

💬 luke-jr commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2211820698)
Concept NACK. This seems to encourage and promote spam. We should be doing the exact opposite.

Curious why you say "creation was already standard" - I don't see why that would be the case.

👍 luke-jr approved a pull request: "wallet: use LogTrace for walletdb log messages at trace level"
(https://github.com/bitcoin/bitcoin/pull/30355#pullrequestreview-2161554757)
utACK 46819f5df6de2a860c3ec87d55527b617a3b128f

💬 luke-jr commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2211824602)
Weak approach NACK. This is just limiting the max block weight, which is already a configurable parameter. Having two params for the same thing seems like a poor way to do it. I'm not sure it avoids the risk of bugs producing invalid blocks, either - it just moves the bug risk from one place to another equally probable.

Instead, maybe just perform the block weight cap on initialization rather than block assembly?

💬 luke-jr commented on pull request "[WIP] net: return result from addnode RPC":
(https://github.com/bitcoin/bitcoin/pull/30381#issuecomment-2211829520)
The return value seems entirely redundant. Maybe it should just be the new peer id (Object-encapsulated for future extension), and make `getpeerinfo` accept a peer id param to just investigate that one?

💬 luke-jr commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#issuecomment-2211848897)
Does this consider the scenario where bitcoin.conf has valid rpcauth params, but the user specifies -norpcauth or such on the command line to disable them?

👍 tdb3 approved a pull request: "test: Remove already resolved assumeutxo todo comment"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2161569821)
ACK 9d5a89509031ce924b9685a6e2e4b19cfef50a9e
Double checking https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2211817861 would also be good as well.

💬 tdb3 commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#issuecomment-2211873385)
> Does this consider the scenario where bitcoin.conf has valid rpcauth params, but the user specifies -norpcauth or such on the command line to disable them?

Thanks for taking a look.
Yes.
`conf_setup()` creates `rpcauth` statements in node 0's bitcoin.conf (including for user `rt`).
After restarting the node with `-norpcauth`, the `assert_equal` with `rt` tests that the associated `rpcauth` statement in the conf is disabled.

https://github.com/bitcoin/bitcoin/blob/1ef15ea7120fe3f04853
...

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667414497)
I think I will refrain from adding these annotations for now, since they are more than just documentation and may have side effects in the generated code. I might experiment with a separate commit to see if they affect performance.

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667414536)
Renamed all `head` to `sentinel`. Hopefully that makes everything clearer :).

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1667415387)
Thanks, still working on reproducing the benchmarks before I can properly ACK it