💬 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

⚠️ ariard opened an issue: "Follow-up from Moderation Rules"
(https://github.com/bitcoin/bitcoin/issues/31110)
Just to be sure this is not a compromised of the github platform itself:
https://github.com/bitcoin-core/meta/issues/11

💬 baycclark commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#issuecomment-2421654737)
> An earlier version allowed users to create watchonly wallets (wallets without private keys) that were "encrypted". Such wallets would have a stored encryption keys, but nothing would actually be encrypted with them. This can cause unexpected behavior such as [bitcoin-core/gui#772](https://github.com/bitcoin-core/gui/issues/772).
>
> We can detect such wallets as they will have the disable private keys flag set, no encrypted keys, and encryption keys. For such wallets, we can remove those en
...

💬 virtu commented on issue "Distribute darknet node addresses via DNS seeds using AAAA records":
(https://github.com/bitcoin/bitcoin/issues/31062#issuecomment-2421667923)
> From what i heard it's not generally a good idea to stash different data in AAAA records. Caching DNS servers can have checks to assume the addresses are routable and valid. Additionally, there's also no guarantee that clients get all the addresses, so splitting the data is a bit brittle.
>
> This would also break the assumption that a random address returned from the DNS seed is connectable (because they might get a fragment instead). Bitcoin core peers fully behind Tor rely on this behavio
...