💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2105116651)
Alright, let's see how it goes with dust limit 1000.

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1597120404)
Note that if we serve this via a BIP157 the cut-through version is going to be indeterministic. Same if people pick a different dust limit. This seems hard to avoid in any case, because the entire filter chain potentially changes with each new block that comes in.

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1597108664)
I worked around this 90 in my pr in https://github.com/bitcoin/bitcoin/pull/29607/files#diff-f146300624c06d2e08aadf500952294148a1785edd6ff2e8b50f13b2c08255edL315 (though I'm not sure why it's not replaced here with `limit`) - I will try to review this in more detail next week

💬 theuni commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1597123552)
Ah, can't believe I missed that. Nice catch!

🤔 furszy reviewed a pull request: "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/28670#pullrequestreview-2050742070)
utACK 521de52c751

💬 furszy commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#discussion_r1597147718)
Shouldn't use `RPC_INTERNAL_ERROR`. As per it description: "this error should only be used for genuine errors in bitcoind (for example datadir corruption)"

Maybe use `RPC_DESERIALIZATION_ERROR` or `RPC_MISC_ERROR.

💬 willcl-ark commented on issue "Slow memory leak in v22.0?":
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2105183958)
Discovered another attempt to avoid copies from @maflcko https://github.com/bitcoin/bitcoin/pull/25429 while researching further.

Ref my previous test in #30052, I have a branch with some improvements (heavily "insipred" by @martinus 's commit https://github.com/jgarzik/univalue/pull/79/commits/e9109e2a04b47472fe45a9cae3c6b41c245a6db9) which limit allocations to about 60-70MB:


This is about
...

🤔 cbergqvist reviewed a pull request: "net: Favor peers from addrman over fetching seednodes"
(https://github.com/bitcoin/bitcoin/pull/29605#pullrequestreview-2050826137)
re-ACK f3c4c4ba894cf919f7dd2d997c3c0c47b320229e

Ran `test/functional/p2p_seednode.py` 15 times to confirm robustness against timeouts.
Also ran functional tests with `-extended --exclude=feature_dbcrash` with no failures.

Still urge you to do something like f9bfc588f2e81a7febe233f591b75e41a52db8b4 from https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1586847044 (still cherry-picks cleanly). Narrowing state carried around to when it is actually used is cleaner and more respectful
...

💬 theuni commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1597205866)
ctrl+f "90", whew that's the only one. This is exactly why named constants are so important to name. I think it's fair to call this a nice cleanup (getting rid of the constant) in addition to the new feature.

💬 theuni commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#issuecomment-2105238891)
ACK retracted until resolving https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1597108664.

📝 TheCharlatan opened a pull request: "kernel: Remove batchpriority from kernel library"
(https://github.com/bitcoin/bitcoin/pull/30083)
The current usage of ScheduleBatchPriority is not transparent. Once the thread scheduling is changed, it remains unchanged for the remainder of the thread's lifetime. So move the call from `ImportBlocks` to the init code where it is clearer that its effect lasts for the entire lifetime of the thread.

Users of the kernel library might not expect `ImportBlocks` to have an influence on the thread it is called in. Particularly since it is only a compile time option and cannot be controlled at run
...

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597235280)
I have done that, but bundled most of the doc changes to a doc-only commit at the beginning in order to not have unrelated things in the "blockstorage: split up FindBlockPos function" commit.

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597235497)
Done as an extra commit at the end.

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#discussion_r1597236294)
I have added this with the latest push. Changed the `// Update the file information so it points to the last block.` comment slightly because the file information doesn't really point to anything.

💬 ryanofsky commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2105282755)
re: https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2083201174

> The `libbitcoin_util` still depends on `libbitcoin_crypto`, `libbitcoin_consensus` and `libbitcoin_common` libraries.

Thanks for checking this. Letting the util library depend on the crypto library seems like the easiest path to take because random.cpp, randomenv.cpp, hasher.cpp, and bytevectorhash.cpp are using the crypto hash functions. But I added new commits to the PR to remove the util dependency on crypt, and
...

💬 mzumsande commented on pull request "blockstorage: Separate reindexing from saving new blocks":
(https://github.com/bitcoin/bitcoin/pull/29975#issuecomment-2105289929)
[6a22eed ](https://github.com/bitcoin/bitcoin/commit/6a22eede2083616ecc7558a16d8189c22b46403d)to [9cf475f](https://github.com/bitcoin/bitcoin/commit/9cf475ffffb869cd55c2b2f3be84d7c90b199521):

I think I addressed all feedback now. Added 2 additional commits (one for renaming / updating FindNextBlockPos, one for not moving the cursor backwards in UpdateBlockInfo) and reorganized the other commit slightly by having a doc-only commit at the beginning.

💬 jonatack commented on pull request "i2p: fix and improve logs":
(https://github.com/bitcoin/bitcoin/pull/29833#discussion_r1597264413)
This along with https://github.com/bitcoin/bitcoin/pull/29833/files#diff-8945a03581b372d674f34dcb6a63e537343e376ebcd9d8c7c3d94d4366cd9b9dL286 appears to be the same change as [`43315a0` (#25203)](https://github.com/bitcoin/bitcoin/pull/25203/commits/43315a0f692ba6e8621a677429c6f8cc6a3dd6cd).

🤔 jonatack reviewed a pull request: "i2p: fix and improve logs"
(https://github.com/bitcoin/bitcoin/pull/29833#pullrequestreview-2050930349)
Concept ACK, this is very similar to the changes in [`fc0a50e` (#25203)](https://github.com/bitcoin/bitcoin/pull/25203/commits/fc0a50e92aaaf3b049fcfcd0873dadc7150cfecf) and [`43315a0` (#25203)](https://github.com/bitcoin/bitcoin/pull/25203/commits/43315a0f692ba6e8621a677429c6f8cc6a3dd6cd), in case you'd like to compare/reference with them.

💬 pablomartin4btc commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2105354864)
> Why?

Discussed a bit offline... Please feel free to add more context here and any concerns you have.

💬 pablomartin4btc commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#issuecomment-2105456511)
Updates:
- [added the call](https://github.com/bitcoin-core/gui/blob/ba13f10f9ca5bad7de0e140b667fbae5e8b7b9a3/src/qt/transactionview.cpp#L675) to `raise()` as @alfonsoromanz [suggested](https://github.com/bitcoin-core/gui/pull/817#discussion_r1569483043) finding that the fix wasn't working properly on Mac without it, this is not causing any other effects on Linux or Windows which I both tested.
- added includes of `QString` in both [`src/qt/transactiondescdialog.h`](https://github.com/bitcoin
...