💬 ryanofsky commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1579587621)
In commit "[[refactor]] Check CTxMemPool options in constructor" (30a1024b63105a97d04149e83ae2d8cf0830cf69)

Would be good to add comment pointing out that error is currently ignored. (And maybe that this ensures mempool code passes fuzz tests when even max mempool size is unreasonably small.)

💬 npslaney commented on issue "Improving fee estimation accuracy":
(https://github.com/bitcoin/bitcoin/issues/27995#issuecomment-2077440875)
Bitcoin's fee estimator is outdated for sure. For lightning channels, where some functionality depends on accurate fee estimation, fee spikes like what we had last week can make small channels that are typically usable unusable, and that effect lasts longer due to bitcoind's fee estimation lagging what it actually takes to get into the next block.

Lightning peers have to agree on fees to an extent to function. We would love to pipe in our own fee estimation, but we are stuck in a lowest commo
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077460938)
Sounds like a bug on our end then. The BIP says:

> outpoint (36 bytes): the `COutPoint` of an input (32-byte txid, least significant byte first || 4-byte vout, least significant byte first)

And there's a footnote saying "Why are outpoints little-endian".

https://github.com/bitcoin/bips/blob/57c89ae162b4dab971dc6061ba6acf7d676781ea/bip-0352.mediawiki#cite_ref-why_little_endian_8-0

We should probably have a BIP352 specific `COutPoint` sorter, and a test vector.

👋 fanquake's pull request is ready for review: "depends: swap some cctools tools for LLVM tools"
(https://github.com/bitcoin/bitcoin/pull/29739)

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579633610)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579447042

Personally I think using lowercase for instance methods and uppercase for other methods and functions is good practice because it makes it obvious when you are passing an implicit `this` parameter to function calls. (It is useful for the same reason `m_` prefixes are useful.) However, I got pushback when I tried to allow this in the style guide in #14635 so now I usually try to follow the guide.

💬 ryanofsky commented on pull request "Disable util::Result copying and assignment":
(https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579632793)
re: https://github.com/bitcoin/bitcoin/pull/29906#discussion_r1579500335

It seems like the goal of the code is to get a new address without unlocking. But I haven't looked into this. It definitely looks a little clumsy, though, because it isn't actually using the error message that is returned.

💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2077491415)
> We should probably have a BIP352 specific `COutPoint` sorter, and a test vector.

Yeah, I'm working on a test vector that incorporates this kind of scenario.

💬 fanquake commented on pull request "depends: build miniupnpc with CMake":
(https://github.com/bitcoin/bitcoin/pull/29707#issuecomment-2077505030)
> FWIW, I found another Windows-specific bug in the upstream: https://github.com/miniupnp/miniupnp/pull/727.

Rebased again, but can pull in this change + fixups on next push

💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579698526)
done

💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579698620)
done

💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#discussion_r1579699392)
Thanks for this hint but I think usage of `pathlib` is usually preferred now because it is more modern. So I will keep this as is unless there are more pros to this change that I have overlooked.

💬 fjahr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-2077562279)
Addressed comments from @virtu , thanks for reviewing!

💬 sr-gi commented on pull request "net: Decrease nMaxIPs when learning from DNS seeds":
(https://github.com/bitcoin/bitcoin/pull/29850#issuecomment-2077565724)
Post-merge ACK f2e3662e57eca1330962faf38ff428a564d50a11

💬 twosatsmaxi commented on pull request "policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)":
(https://github.com/bitcoin/bitcoin/pull/29843#issuecomment-2077629808)
Following.

📝 theStack opened a pull request: "refactor: remove remaining unused code from cpp-subprocess"
(https://github.com/bitcoin/bitcoin/pull/29961)
This PR removes remaining code that is unused within the cpp-subprocess module (templates and commented out code). Happy to add more removals if anyone finds more unused parts. Note that there are some API functions of the `Popen` class that we don't use, e.g. `wait()`, `pid()`, `poll()`, `kill()`, but they sound IMHO common enough to be useful in the future, so not sure how deep we should go there.

🤔 tdb3 reviewed a pull request: "contrib: rpcauth.py - Add new option (-json) to output text in json format"
(https://github.com/bitcoin/bitcoin/pull/29433#pullrequestreview-2022987855)
Thanks for squashing commits.
ACK for 9adf949d2aa6d199b85295b18c08967395b5570a

💬 hebasto commented on pull request "refactor: remove remaining unused code from cpp-subprocess":
(https://github.com/bitcoin/bitcoin/pull/29961#issuecomment-2077685614)
Concept ACK.

💬 achow101 commented on pull request "lint: scripted-diff verification also requires GNU grep":
(https://github.com/bitcoin/bitcoin/pull/29689#issuecomment-2077705790)
ACK 3bf4f8db669e1e274ce2633cf84add2938b9914b

🚀 achow101 merged a pull request: "lint: scripted-diff verification also requires GNU grep"
(https://github.com/bitcoin/bitcoin/pull/29689)

🤔 brunoerg reviewed a pull request: "net: Make AddrFetch connections to fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/26114#pullrequestreview-2023068219)
Just tested with `-nodnsseed -blocksonly -debug=net` with an empty `peers.dat` and checked the behaviour is as expected. Some logs:

1. Adding fixed seeds
```
2024-04-25T13:02:59Z Adding fixed seeds as -dnsseed=0 (or IPv4/IPv6 connections are disabled via -onlynet) and neither -addnode nor -seednode are provided
2024-04-25T13:02:59Z [net] Added hardcoded seed: 1.253.159.19:8333
2024-04-25T13:02:59Z [net] Added hardcoded seed: 2.152.74.211:8333
2024-04-25T13:02:59Z [net] Added hardcoded se
...

💬 achow101 commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2077749583)
ACK cb8f83df1916308013a6ae4b8e4914eb805c5343