💬 jonatack commented on pull request "script, test: python typing and linter updates":
(https://github.com/bitcoin/bitcoin/pull/28009#issuecomment-1613981755)
Per lint ci https://cirrus-ci.com/task/4666772440219648

```
++ test/lint/all-lint.py
Success: no issues found in 268 source files
```

Ready for review.

👋 jonatack's pull request is ready for review: "script, test: python typing and linter updates"
(https://github.com/bitcoin/bitcoin/pull/28009)

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1247323044)
In commit "index: verify blocks data existence only once" (5ec5a4888743ce7261e0e9cdc077014cd47bfdd6)

Noticed this while I was rebasing #24230, but one side effect of this commit is now `CustomInit` will be called unnecessarily when the index can't start up because there is a pruning violation. This doesn't seem ideal, but it probably not worth worrying about. It would be good to mention the change in the commit description, though.

I think #24230 will restore previous behavior, and not cal
...

💬 luke-jr commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247319876)
```
src/test/logging_tests.cpp:262:29: warning: loop variable 's' of type 'const std::string&' {aka 'const std::__cxx11::basic_string<char>&'} binds to a temporary constructed from type 'const char* const' [-Wrange-loop-construct]
262 | for (const std::string& s : {"", "NONE", "net", "all", "1"}) {
| ^
src/test/logging_tests.cpp:262:29: note: use non-reference type 'const std::string' {aka 'const std::__cxx11::basic_string<char>'} to make the copy expl
...

💬 luke-jr commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247331119)
Kinda ugly to call `node.logging()` repeatedly

💬 luke-jr commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247331674)
Why aren't libevent/leveldb being excluded here?

💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247340446)
Good point, updated.

```diff
# In addition to net and tor, leveldb/libevent/rand are excluded by the test framework.
+ result = node.logging()
for category in ["leveldb", "libevent", "net", "rand", "tor"]:
- assert not node.logging()[category]
+ assert not result[category]
```

💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247342599)
Thank you, Luke, updated. Is that with GCC? I don't see the warning with Clang 16.0.6.

```diff
BOOST_FIXTURE_TEST_CASE(logging_IsNoneCategory, LogSetup)
{
- for (const std::string& s : {"none", "0"}) {
+ for (const char* const& s : {"none", "0"}) {
BOOST_CHECK(LogInstance().IsNoneCategory(s));
}
- for (const std::string& s : {"", "NONE", "net", "all", "1"}) {
+ for (const char* const& s : {"", "NONE", "net", "all", "1"}) {
BOOST_CHECK(!LogInstance
...

💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247357530)
Because `-debugexclude=none"` is passed. It looks like the passed extra args are concatenated to `self.args` before starting the process, i.e. `self.process = subprocess.Popen(self.args + extra_args...` in `test_framework/test_node.py#L219`, so the "none" value cancels the -debugexclude={libevent,leveldb,rand} args in the same file.

💬 jonatack commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1247357713)
Good question, though. I'll add a comment.

💬 MarcoFalke commented on pull request "refactor: Open file in FileCommit":
(https://github.com/bitcoin/bitcoin/pull/28006#issuecomment-1614159773)
Closing for now, until there is a higher need for it.

✅ MarcoFalke closed a pull request: "refactor: Open file in FileCommit"
(https://github.com/bitcoin/bitcoin/pull/28006)

💬 MarcoFalke commented on pull request "fix: delay in TimeOffset applied to AdjustedTime caused by send/receive message queues, correct pointer alignment issue":
(https://github.com/bitcoin/bitcoin/pull/28010#issuecomment-1614167569)
No need to open several pulls for the same change. Let's discuss the concept in #28007 first, which you can then reopen and (force) push to.

✅ MarcoFalke closed a pull request: "fix: delay in TimeOffset applied to AdjustedTime caused by send/receive message queues, correct pointer alignment issue"
(https://github.com/bitcoin/bitcoin/pull/28010)

🤔 MarcoFalke reviewed a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-1506562478)
(nit)

💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1247469065)
```suggestion
throw std::ios_base::failure("AutoFile::seek: file handle is nullptr");
```

📝 MarcoFalke opened a pull request: "test: Rename EncodeDecimal to serialization_fallback"
(https://github.com/bitcoin/bitcoin/pull/28011)
The new name better explains that the function handles fallbacks, without listing all in the function name.

👍 MarcoFalke approved a pull request: "util: Show descriptive error messages when FileCommit fails"
(https://github.com/bitcoin/bitcoin/pull/26654#pullrequestreview-1506634475)
nice. lgtm, but I didn't look at the windows stuff

💬 MarcoFalke commented on pull request "util: Show descriptive error messages when FileCommit fails":
(https://github.com/bitcoin/bitcoin/pull/26654#discussion_r1247515634)
```suggestion
LogPrintf("fsync failed: %s\n", SysErrorString(errno));
```

nits:

* Can remove the manual func logging, which isn't needed, because all strings in this function are already unique to the codebase. Also, users can enable the setting which enabled function logging.
* Can change `%d` to `%s`, since it is a string. (No behavior difference though)

💬 FelixWeis commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1614300636)
reverted to compiling 6ee3881551f2cd411c4e4d8b0ccedf0f0416d8c2 (tag: v25.0). same cpu, os, compiler version. runs smooth since 3 days