💬 TheBlueMatt commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1804784541)
Shouldn't this return a blocktemplate instead? Requiring a second round-trip after getting a notification is just unnecessary latency.

💬 TheBlueMatt commented on pull request "Add waitFeesChanged() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31003#discussion_r1804793319)
It'd be nice if `bitcoin.conf` could specify some options to restrict the fee threshold/timeout to some window. That way (once we drop CNB from the interface, see #31109) the protocol is at least kinda robust against trivial DoS attacks, which would be nice for teeing it up to be served over TCP in the future (or the inevitable exposing it over netcat that'll happen).

🤔 darosior reviewed a pull request: "validation: Improve input script check error reporting"
(https://github.com/bitcoin/bitcoin/pull/31097#pullrequestreview-2375362753)
re-ACK 86e2a6b749c7fecbd086b361806ac9f6e9426d79

💬 jonatack commented on pull request "rpc: net: follow-ups for #30062":
(https://github.com/bitcoin/bitcoin/pull/30183#discussion_r1804904158)
"at the end of the BGP route to the peer" in all of them (including -netinfo as well) seems good

💬 danielabrozzoni commented on pull request "rest: Support transaction broadcast in REST interface":
(https://github.com/bitcoin/bitcoin/pull/31065#issuecomment-2419741359)
> Worth adding a release note?

Thank you! Done in 6b32a886e351f0af3084ccab076a52e0eafc5aab

@stickies-v:

> It seems electrs already uses the authentication cookie by default (as per their docs), so for most setups, this shouldn't be a meaningful difference?

I agree that adding this endpoint won't make much difference in the short term, but I still believe that in the long term, if both existing and new indexers adopt the REST interface, it will improve UX and security.

> It seems t
...

🚀 fanquake merged a pull request: "tracing: explicitly cast block_connected duration to nanoseconds"
(https://github.com/bitcoin/bitcoin/pull/29877)

💬 maflcko commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1805079799)
This fails ubsan:


```
src/bench/crypto_hash.cpp:202:9: runtime error: implicit conversion from type 'int' of value 256 (32-bit, signed) to type 'unsigned char' changed the value to 0 (8-bit, unsigned)
#0 0x55b763589f3f in SipHash_32b(ankerl::nanobench::Bench&)::$_0::operator()() const bld-cmake/src/bench/./src/bench/crypto_hash.cpp:202:9


...

SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation /home/marco/workspace/b-c/src/bench/crypto_hash.cpp:202:9
```

...

🤔 BrandonOdiwuor reviewed a pull request: "init: Some small chainstate load improvements"
(https://github.com/bitcoin/bitcoin/pull/31046#pullrequestreview-2376140194)
Code Review ACK 31cc5006c3de4dd6a1f7a238684163956604df45.

Nice work on the chainstate loading improvements, especially the enhanced handling of database-specific errors.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1805421857)
Updated to take @vasild's proposal. I believe it's the best path of the 3 options discussed above.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1805423837)
Didn't take, as those braces are unneeded due to the low precedence of the ternary operator, clang-format doesn't prefer them, and they add clutter. Will add if people insist.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2420560896)
> I think you also should update line 93 (change "4" to "5")
>
> ```
> argsman.AddArg("-netinfo", "Get network peer connection information from the remote server. An optional integer argument from 0 to 4 can be passed for different peers listings (default: 0). Pass \"help\" for detailed help documentation.", ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
> ```

Good catch! added 153b898a77f4c892731c220ce85d29c1cf880355 to ensure we don't overlook updating this.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2420562022)
> I think you also should update line 93 (change "4" to "5")
>
> ```
> argsman.AddArg("-netinfo", "Get network peer connection information from the remote server. An optional integer argument from 0 to 4 can be passed for different peers listings (default: 0). Pass \"help\" for detailed help documentation.", ArgsManager::ALLOW_ANY, OptionsCategory::CLI_COMMANDS);
> ```

Good catch @LarryRuane -- added 153b898a77f4c892731c220ce85d29c1cf880355 to ensure we don't overlook updating this.

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2420580314)
Updated per `git diff 683b558 1847b9` to take @vasild's [suggestion](https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1795748339) and a fix for @LarryRuane's [catch](https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2363290423) (thanks!)

💬 jonatack commented on pull request "netinfo: add peer services column and outbound-only peers list":
(https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2420629857)
Updated to take @vasild's [suggestion](https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1795748339) and a fix for @LarryRuane's [catch](https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2363290423) (thanks!) and doc updates I overlooked.

<details><summary><code>git diff 683b558 17f8f03</code></summary><p>

```diff
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 000f7695de6..6bda4cbe0c5 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -57
...

🤔 ariard reviewed a pull request: "validation: Improve input script check error reporting"
(https://github.com/bitcoin/bitcoin/pull/31097#pullrequestreview-2376369218)
Re-Code Review ACK 86e2a6b7

The code nit improvement, ready to be git cherry-picked:https://github.com/ariard/bitcoin/tree/2024-10-check2-impr

💬 masahiro0000 commented on issue "CI failure: `Error: Directory '/tmp/ccache_dir' must be created in advance.`":
(https://github.com/bitcoin/bitcoin/issues/31108#issuecomment-2420879801)
Hi @maflcko
In commit fa252da, the CCACHE_DIR was removed from the .cirrus.yml file. Originally, the directory for CCACHE_DIR is supposed to be created in another shell script. However, since the definition has been deleted, the directory is not being created, which is likely causing the error.

As a solution, it seems that the previous commit might have been incorrect, so we may need to contact the administrator and request them to revise the commit.

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2420893629)
@mzumsande

> Again, the issues mentioned in #30572 (comment) and #30572 (comment) point out that unrequested transactions are received today when an orphan resolution clears outstanding tx requests both by txid and wtxid here, but there is still> a tx request in flight (we already sent GETDATA but haven't received the TX yet), which is seen as unrequested when the TX arrives. So the transactions were requested before, but aren't seen as requested anymore at the point the tx > is received. Ho
...

👍 1440000bytes approved a pull request: "rest: Support transaction broadcast in REST interface"
(https://github.com/bitcoin/bitcoin/pull/31065#pullrequestreview-2376591484)
ACK https://github.com/bitcoin/bitcoin/pull/31065/commits/6b32a886e351f0af3084ccab076a52e0eafc5aab

Two changes since last review:
- [Test](https://github.com/bitcoin/bitcoin/pull/31065#discussion_r1799005090) fixed
- Added in [release notes](https://github.com/bitcoin/bitcoin/commit/6b32a886e351f0af3084ccab076a52e0eafc5aab)

> Do you think it would make sense to add an additional flag, like -allowrestbroadcast, defaulting to off?

I don't think this needs to be done in bitcoin core. Web
...

📝 andrewtoth converted_to_draft a pull request: "Don't wipe coins cache when full and instead evict LRU clean entries"
(https://github.com/bitcoin/bitcoin/pull/31102)
Depends on #28939.

This only wipes the coins cache if memory usage is greater than total allowed cache size. Instead, it does a non-wiping sync to disk and keeps all unspent coins in the cache. These are tracked in a linked list of clean entries, and when spent are removed from the clean linked list and appended to the dirty linked list. This results in the head of the clean linked list containing the oldest clean entry.

When the cache grows to above the large size threshold, clean entries
...

👍 1440000bytes approved a pull request: "netinfo: add peer services column and outbound-only peers list"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2376596938)
ACK https://github.com/bitcoin/bitcoin/pull/30930/commits/17f8f03ec69b6dd3d61137a39b8f88201ec500dc

Changes since last review: https://github.com/bitcoin/bitcoin/pull/30930#issuecomment-2420629857