💬 sdaftuar commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1553666857)
> Having the number of allowed evictions be a function of the descendant limit makes replacements easier to reason about from a pinning perspective. e.g., limit of 25/100 means that you can batch CPFP 4 transactions safely, ignoring other pinning vectors(we'll get around to those!). Another smaller point is currently people opting into larger descendant limits for whatever reason can cause this to fall to possibly 0. i.e. a chain of 101 txns that can't have eldest tx evicted.

Thanks for menti
...

💬 ismaelsadeeq commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1553669447)
> https://github.com/bitcoin-core/bitcoin-devwiki/wiki/25.0-Release-Candidate-Testing-Guide#test-finalizing-a-psbt-with-inputs-spending-miniscript-compatible-p2wsh-scripts-and-test-spending-the-coin
>
> Is the explanation of the descriptor correct?

Sure that is the correct explanation, I got the same output from https://bitcoin.sipa.be/miniscript/
Miniscript input `or_d(pk(A),and_v(v:pk(B),and_v(v:pk(C),older(10))))`
Script output
```
<A> OP_CHECKSIG OP_IFDUP OP_NOTIF
<B> OP_CHECKSI
...

👍 pablomartin4btc approved a pull request: "Debug Console implementation of generate method"
(https://github.com/bitcoin-core/gui/pull/692#pullrequestreview-1433505009)
Liked the refactoring and the 2 commits split as @hebasto requested.
Tested ACK both commits separately, also checked also other RPC commands, it works as expected.

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