💬 tdb3 commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#issuecomment-2118676930)
@jonatack @kevkevinpal, what are your thoughts on reopening PR #30106 and dropping the test runner `insuffient` spelling error from 30084?

Seems like this PR (30084) is scoped to more than just the spelling error and fixing the error could help prevent near term lint issues for open PRs.

Maybe I'm missing something and this was already addressed?

📝 fanquake unlocked a pull request: "lint: fixed typo in test_runner causing linter warning"
(https://github.com/bitcoin/bitcoin/pull/30106)
introduced in https://github.com/bitcoin/bitcoin/commit/357ad110548d726021547d85b5b2bfcf3191d7e3

This typo is causing an error in the linter, fixing this should remove this warning

```
test/functional/test_runner.py:651: insuffient ==> insufficient
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
```

[link to cirrus ci warning](https://cirrus-ci.com/task/5926490804584448?logs=lint
...

💬 laanwj commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1605695254)
Agree! i mean in a hypothetical future (say, C++30) when every compiler and OS finally implements thread-local storage correctly including destructors, it could be reconsidered, but i think a "no-thread-local" policy makes sense until then.

TBH in general i don't really like comments that suggest TODOs. It's better to keep track of those things in issues where discussion and context is visible.

💬 laanwj commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#issuecomment-2118681020)
Code review ACK d35ba1b3f16071b8fe9b36398ba15352dbf2a54d

💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1605695740)
I ended up deleting all of this in #30132.

💬 maflcko commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605700154)
Instead of claiming that this was suggested to be changed by a linter, it would be better to have a real motivation for the change, as well as a reason why the change is correct.

Looking at this, I don't see why continuing is the right behavior change here.

💬 maflcko commented on pull request "lint/contrib/doc updates":
(https://github.com/bitcoin/bitcoin/pull/30084#discussion_r1605699782)
nit: Shouldn't this go into the section that ends with "...could be outdated" above?

💬 maflcko commented on issue "LevelDB error: Corruption: block checksum mismatch didn't trigger reindex.":
(https://github.com/bitcoin/bitcoin/issues/30138#issuecomment-2118691502)
Can you clarify this? The debug log says "Shutdown: done", but you claim that "It connects to many peers", indicating that the program is still running?

If this is the case, it would be a bug, because the program should abort and shutdown when file corruption is detected in the leveldb database.

💬 jadijadi commented on pull request "Showing Local Addresses in Node Window":
(https://github.com/bitcoin-core/gui/pull/626#issuecomment-2118694174)
> This PR still have a couple of unaddressed comments: [#626 (comment)](https://github.com/bitcoin-core/gui/pull/626#discussion_r915334998) and [#626 (comment)](https://github.com/bitcoin-core/gui/pull/626#discussion_r1274077350).
>
> @jadijadi Are you still working on this?

Will check and comment soon

💬 maflcko commented on pull request "fuzz: add more coverage for `ScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/30134#discussion_r1605702378)
Can you explain this change?

```
wallet/test/fuzz/scriptpubkeyman.cpp should add these lines:
#include <assert.h> // for assert
#include <functional> // for function
#include <map> // for map
#include <memory> // for unique_ptr, shared_ptr, make_unique, __shared_ptr_access
#include <optional> // for optional, nullopt, nullopt_t
#include <unordered_set>
...

💬 maflcko commented on pull request "test: remove unneeded `-maxorphantx=1000` settings":
(https://github.com/bitcoin/bitcoin/pull/30133#issuecomment-2118697981)
utACK 8950053636cb38ed85fe2d58b53e5d0acb35c390

💬 maflcko commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#issuecomment-2118706781)
Not sure what's wrong with the CI ( https://github.com/bitcoin/bitcoin/actions/runs/9133680294/job/25117694444?pr=30132#step:6:2 ) Maybe rebase again?

💬 brunoerg commented on pull request "fuzz: add more coverage for `ScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/30134#discussion_r1605721747)
Nevermind, just dropped this change.

💬 rkrux commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605724296)
Hmm I see now what you mean.

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1605724571)
Sure, but then the test will fail regardless. Might as well fail early, if the peer disconnects again, no?

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2118731144)
re-ACK 09ef322acc0a88a9e119f74923399598984c68f6 . Fixed unreachable assert and updated docstring, and also added an exception for "-maxmempool must be at least " in the `tx_pool` fuzz test, which makes sense when looking at how the mempool options are constructed in `SetMempoolConstraints`.

👍 rkrux approved a pull request: "test: Remove struct.pack from almost all places"
(https://github.com/bitcoin/bitcoin/pull/29401#pullrequestreview-2064681964)
tACK [fa52e13](https://github.com/bitcoin/bitcoin/pull/29401/commits/fa52e13ee81fcc7543890dbd6986fcb55168583f)

Make successful, so are all the functional tests.

Overall I agree with the intent to get rid of `struct.pack` usage, which requires looking up the documentation most of the time and might be prone to errors. Using the alternate approach of `to_bytes` is more self-documenting and preferable to me.

📝 hebasto opened a pull request: "build: Disable `_FORTIFY_SOURCE` if using MSan"
(https://github.com/bitcoin/bitcoin/pull/30140)
This PR shifts responsibility to disable `_FORTIFY_SOURCE` from the user to the build system, which:
- improves usability
- simplifies the CI and OSS-Fuzz scripts
- is useful for CMake migration as it removes the need to use non-standard `APPEND_CPPFLAGS` in such a corner case

💬 fanquake commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118764778)
Concept NACK. If anything should be changed, it would be the implementation of applying the hardening flags in the CMake branch, so it's possible to override them using standard CMake variables. Not push more complexity into the build system, and hardcode specific things.

Removing APPEND_*FLAGS shouldn't be a goal, as it needs to exist, to do what it exists for (actually give a mechanism for always overriding a flag, regardless of what CMake try's to do). Otherwise in future, if we (or any oth
...

💬 hebasto commented on pull request "build: Disable `_FORTIFY_SOURCE` if using MSan":
(https://github.com/bitcoin/bitcoin/pull/30140#issuecomment-2118770672)
> Concept NACK. If anything should be changed, it would be the implementation of applying the hardening flags in the CMake branch, so it's possible to override them using standard CMake variables.

I disagree. The current implementation is undocumented and error-prone on the user side.

> Not push more complexity into the build system, and hardcode specific things.

This statement seems quite arbitrary. Considering that the build system is full of "hardcode specific things" that "push mor
...