💬 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
...

💬 pablomartin4btc commented on pull request "assumeutxo, rpc: Improve EOF error when reading snapshot metadata in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/28670#discussion_r1597331838)
It makes sense, I'll fix it, thanks!

📝 jonatack opened a pull request: "script: lief updates"
(https://github.com/bitcoin/bitcoin/pull/30084)
Fix the following linter errors observed since recent releases of lief (see its [changelog](https://lief.re/doc/latest/changelog.html))

```
contrib/devtools/symbol-check.py:269: error: Module has no attribute "NOTE_TYPES" [attr-defined]
contrib/devtools/symbol-check.py:270: error: Module has no attribute "NOTE_ABIS" [attr-defined]
contrib/devtools/symbol-check.py:274: error: Module has no attribute "EXE_FORMATS" [attr-defined]
contrib/devtools/symbol-check.py:281: error: Module has no
...

💬 fanquake commented on pull request "script: lief updates":
(https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2105498216)
> Fix the following linter errors observed

There are currently no linter errors if you [run the linter using the container](https://github.com/bitcoin/bitcoin/tree/master/test/lint), which installs the same versions as the CI. This is the recommended way to run the linters, as it avoids differences that may occur due to different versions of dependencies installed on your local system, and the need for changes like this.

> since recent releases of lief

As LIEF mostly exists here for the
...

💬 jonatack commented on pull request "script: lief updates":
(https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2105505084)
This patch allows the individual linters to continue to be usefully called without Docker or Rust, per the following section of `test/lint/README.md`:

```
#### Running the tests

Individual tests can be run by directly calling the test script, e.g.:

test/lint/lint-files.py
```

I've been running them that way for 5+ years and would prefer to be able to continue doing so. If we now no longer want to support calling them directly or individually, then I'll update the README to disc
...

👋 jonatack's pull request is ready for review: "script: lief updates"
(https://github.com/bitcoin/bitcoin/pull/30084)

🤔 tdb3 reviewed a pull request: "Add option dbfilesize to control LevelDB target ("max") file size"
(https://github.com/bitcoin/bitcoin/pull/30059#pullrequestreview-2051068940)
Concept ACK.

Thank you. Seems reasonable to include this as a debug option, and increases flexibility over the single value chosen for PR #30039. Left an inline comment/question.

Ran a quick sanity check:
With `dbfilesize=64` set (along with txindex=1), performed an IBD on Signet from a node on the same LAN, then stopped and restarted bitcoind. Saw chainstate and indexes/txindex files that were close to 64MB (blocks/index for signet was less than 64MB, but still had a file over 2MB).

...

💬 tdb3 commented on pull request "Add option dbfilesize to control LevelDB target ("max") file size":
(https://github.com/bitcoin/bitcoin/pull/30059#discussion_r1597350392)
The text states a range of 1MiB to 1024MiB, but these values didn't seem to be enforced (e.g. 0.5, -3, and 1025 were not rejected). Is the intent here to provide guidance for the user (i.e. tell the user to choose 1 to 1024 MiB) rather than enforce a specific range of values?

👍 theStack approved a pull request: "refactor: Remove unused code from `subprocess.h` header"
(https://github.com/bitcoin/bitcoin/pull/30081#pullrequestreview-2051074164)
Code-review ACK 5a11d3023f7d0cde777f3496c0f3aa381823d749

Thanks for following up!

👍 theStack approved a pull request: "crypto: add `NUMS_H` const"
(https://github.com/bitcoin/bitcoin/pull/30048#pullrequestreview-2051074508)
re-ACK 56b8f5e163465673e329f77f6cfcff3f0a42d533