✅ 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

👍 luke-jr approved a pull request: "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin"
(https://github.com/bitcoin/bitcoin/pull/30326#pullrequestreview-2161585202)
utACK 00052ecc0402f2c53e3602b901a937b64aa7f7cc

There's a potential race condition for negative lookups, but I assume it's fine since the old code would also have a (different) race.

Performance might be slightly worse for negative lookups, but that seems an acceptable tradeoff since the positive lookup path is much more likely, especially during IBD.