💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1198355535)
Thanks for your review, I have fixed it.

💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1553741863)
It took several days to track down, but... I believe the issue with clang >=11 comes from: https://github.com/llvm/llvm-project/commit/6fa3894c4e771c773712b1ae777f78c1c922a908 . From my local tests I'm able to revert that and get back to expected results.

So I guess we'll need to patch it out of guix.

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1198379250)
Still a nub on cpp,
`constexpr auto FEE_FLUSH_INTERVAL{1h};` does not compile
I added instead `static constexpr std::chrono::hours FEE_FLUSH_INTERVAL{1};`

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1198379499)
Thanks fixed

💬 ismaelsadeeq commented on pull request "Fee estimation: avoid serving stale fee estimate":
(https://github.com/bitcoin/bitcoin/pull/27622#discussion_r1198388360)
>Mine as well use this constant here for the filename?

:thinking: it is an anon namespace, not defined in policy/fees_args.h,
I think it is okay as is, or else

policy/fees_args.h
```
const char* GetFeeEstimatesFilename();
```
policy/fees.args.cpp
```
const char* GetFeeEstimatesFilename() {
return FEE_ESTIMATES_FILENAME;
}
```
policy/fees.cpp
```
LogPrintf("Flushed fee estimates to %s.\n", GetFeeEstimatesFilename());
```
but if it's possible another way I will be glad
...

💬 sdaftuar commented on pull request "validation: have LoadBlockIndex account for snapshot use":
(https://github.com/bitcoin/bitcoin/pull/23174#discussion_r1198450499)
Sorry for chiming in on what is now a very old PR, but I believe this block of code is incorrect. If we have a snapshot in place (and so some block index is marked IsAssumedValid), but we have downloaded blocks that haven't yet been activated (eg on the background-validation chainstate), this logic prevents us from adding those downloaded-but-not-yet verified blocks to `setBlockIndexCandidate` for the background chainstate.

I ran into this while working on a download implementation for assum
...

💬 ajtowns commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1553973702)
Maybe we can move the boost option into a new `--enable-slow-debug` option, and just enable that for fuzzing and perhaps one of the CI tasks?

```diff
diff --git a/configure.ac b/configure.ac
index cbbf8d6172..fbf7a72e7b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -306,6 +306,13 @@ AC_ARG_ENABLE([debug],
[enable_debug=$enableval],
[enable_debug=no])

+dnl Enable slow debug
+AC_ARG_ENABLE([slow-debug],
+ [AS_HELP_STRING([--enable-slow-debug],
+ [
...

💬 MarcoFalke commented on pull request "random: drop syscall wrapper usage for getrandom()":
(https://github.com/bitcoin/bitcoin/pull/27699#discussion_r1198582434)
This function is now unused?

💬 MarcoFalke commented on pull request "random: drop syscall wrapper usage for getrandom()":
(https://github.com/bitcoin/bitcoin/pull/27699#discussion_r1198583011)
```suggestion
}
// Silence a compiler warning about unused function.
(void)GetDevURandom;
```

💬 MarcoFalke commented on issue "CPU DoS on mainnet in debug mode":
(https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1554063021)
See also https://github.com/bitcoin/bitcoin/pull/27300#pullrequestreview-1352821536

💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1554067822)
> Needs rebase on current master, if still relevant

I believe this PR is still very relevant because it substantially improves the logic around address decoding and specifically the displaying of errors in the GUI and RPC. With that said I have rebased and this code is passing all tests other than `Win64 native [vs2022]` which seems to be failing for the majority of PRs that are on master due to warnings.

💬 MarcoFalke commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1554070439)
Thanks, the reason I mentioned it was the silent merge conflict: `key_io.cpp:152:48: error: ‘const class CChainParams’ has no member named ‘NetworkIDString’`, which is now fixed

💬 russeree commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#issuecomment-1554074157)
> Thanks, the reason I mentioned it was the silent merge conflict: `key_io.cpp:152:48: error: ‘const class CChainParams’ has no member named ‘NetworkIDString’`, which is now fixed

Is there a good way to detect these silent conflicts earlier? Usually I get notified via email when there are issues that need rebased; this is the first time one has happened without a notification.

Thanks

💬 ajtowns commented on pull request "build: debug enable addrman consistency checks":
(https://github.com/bitcoin/bitcoin/pull/27300#discussion_r1198630607)
If these checks are going to be enabled by default on some builds, shouldn't the default be much higher; eg 100 or 1000? Doing it on every call is useful for live debugging or small scale tests, but seems overkill for a node's runtime default?

💬 ajtowns commented on issue "Enable consistency checks by default with `--enable-debug`":
(https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1554148356)
Not sure this is a good idea? If the consistency check is runtime configurable, why not just configure it at runtime -- anyone who's going to actively debug any problems found isn't going to find it too hard to add a line in bitcoin.conf? If the problem is just that there might be lots of different options and someone might want to get all of them, wouldn't adding an `allchecks=1` option be better? (Or `-checks=addrman -checks=mempool -checks=all` similar to the `-debug=*` option)

The drawbac
...

💬 willcl-ark commented on pull request "build: debug enable addrman consistency checks":
(https://github.com/bitcoin/bitcoin/pull/27300#discussion_r1198635826)
Yes I agree, the performance hit is too much for any kind of normal operation.

I updated the branch with @MarcoFalke's suggestion along with a new debug option (which I called `--enable-debug-checks`, and not `--debug-slow`).

I'll run a few runtime tests on various values for the checks to see if I can find a sweet spot for the frequency, then update the doc commit and push an update here.

💬 MarcoFalke commented on issue "Enable consistency checks by default with `--enable-debug`":
(https://github.com/bitcoin/bitcoin/issues/24709#issuecomment-1554170480)
I think it is pretty clear that we'll have to add at least one more configure flag for (expensive) debug checks. See `--enable-slow-debug` (https://github.com/bitcoin/bitcoin/issues/27586#issuecomment-1553973702) and `--debug-mode` (https://github.com/bitcoin/bitcoin/pull/27300#issuecomment-1479890611). So I fail to see the downside of putting more (runtime configurable) consistency checks into that new flag.

Regardless, it is already questionable whether `--enable-debug` is suitable for prod
...

👍 MarcoFalke approved a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561#pullrequestreview-1433948180)
lgtm

💬 MarcoFalke commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1198655070)
what is the point of changing this? I'd say to either leave as-is or change to Pathlib, which can use the `/` operator?

💬 MarcoFalke commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1554191949)
Looks like the tests fail in CI, even on a re-run?