💬 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

💬 MarcoFalke commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1614306854)
> runs smooth since 3 days

This implies it is not an upstream bug, or the issue fixed itself in the meantime. This may take some time, but you could try bisecting?

💬 TheCharlatan commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1247595124)
I can't think of a good reason why this shouldn't be `[[nodiscard]]` too. However, as I said [here](https://github.com/bitcoin/bitcoin/pull/27866#discussion_r1246307365) I am trying to keep this PR's scope limited to just the flush functions and their immediate call sites. I added the `[[nodiscard]]` to `FindBlockPos`, because it reports on the flush error now too. `FindUndoPos` on the other hand is not immediately affected by the flush errors. There are many such examples where we ignore error
...