🤔 ismaelsadeeq reviewed a pull request: "rpc: validate fee estimation mode case insensitive"
(https://github.com/bitcoin/bitcoin/pull/29175#pullrequestreview-1829533448)
Code review ACK 51fdb4819ebda4d4a8f64c8d36a471af65a033c9

Just a single nit, happy to re ACK when fixed.

I've tested this on regtest and it passes as expected.

💬 ismaelsadeeq commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#discussion_r1457349966)
nit:
iwyu
```cpp
#include <util/strencodings.h>
```

💬 ismaelsadeeq commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-1898369428)
> I believe CI failures are unrelated. Is it possible to force CI to re-run?

It has to do with #29234, If you rebase on master I think the CI will be happy, but IMO there is no need since it's unrelated

👍 ismaelsadeeq approved a pull request: "fuzz: make FuzzedDataProvider usage deterministic"
(https://github.com/bitcoin/bitcoin/pull/29043#pullrequestreview-1829550326)
ACK 01960c53c7d71c70792abe19413315768dc2275a

💬 dergoegge commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1457369802)
Circling back to this, I still don't think the interface choices in this PR are right.

Mostly it's just that the `NetEventsInterface` is not the place for a getter like this. It is meant for networking events (hence the name) such as opening/closing connections and processing/sending messages. So Approach NACK on misusing this interface.

> The node's contextual information and logic required to take the decision of opening a socket to a particular address resides mostly in PeerManager (rig
...

💬 1440000bytes commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898392767)
> Which could then be discarded by node operators, demonstrating its purpose.

It wont be discarded by default because they use less than 80 bytes for JSON. Could use even lesser bytes by changing the protocol.

> The 80 byte limit on OP_RETURN is a filter, it is one which can be bypassed simply by having a miner include transactions which exceed it out of band. Why is this not deemed "censorship" and why does bitcoin core bother placing an 80 byte limit on it?

Related pull request and a
...

💬 Retropex commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898413591)
> It wont be discarded by default because they use less than 80 bytes for JSON

You persist in not understanding that the goal is not only to protect the network from spam but also to give the node runners the tools to choose their mempool policies.

💬 1440000bytes commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898437260)
> > It wont be discarded by default because they use less than 80 bytes for JSON
>
> You persist in not understanding that the goal is not only to protect the network from spam but also to give the node runners the tools to choose their mempool policies.

If some users really want such options, I had suggested an alternative approach in this comment: https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1854442040

OR

You can add another config option which is enabled by default a
...

💬 joeyvee1986 commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1898442385)
this has seriously been a nonstop threat for me. i wish i could i could
more people to help me wipe all the bugs off this network because im
finding all kinds of weird code scattered throughout my network as i learn
more and more things. pretty sure someone either has some type of immutable
session cookie of mine or i never fully recovered after being sim swapped
last year.

On Fri, Jan 5, 2024 at 10:25 AM Luke Dashjr ***@***.***>
wrote:

> The datacarriersize policy option is meant to
...

💬 instagibbs commented on issue "Timeout downloading block":
(https://github.com/bitcoin/bitcoin/issues/12291#issuecomment-1898473594)
@ArmchairCryptologist matches expectations. Any single stalling peer at chaintip should not stall node completely once your node is "warmed up" with compact block "high bandwidth" connections.

💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898474891)
> > What is stopping the demand for P2MS from moving to P2PK?
>
> Or P2WSH

Or just just the script pubkey in general. I did a small test on testnet where I putt a dickbutt.jpg into the hashes of a witness program.

https://mempool.space/testnet/tx/9382f1504e8381a09773a7eeb9fd2e52c454cdd7263db155f0417a97f7d77c52

🤔 stickies-v reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1829683082)
Post-merge re-ACK df30247705940c50c5eaafd74e2abbeb8b0cec07, I was still investigating the non-deterministic behaviour in `wallet_import_rescan` but I've now found the issue and it doesn't seem problematic enough to revert or immediately fix.

When rebasing these tests on top of e.g. https://github.com/bitcoin/bitcoin/pull/29019 as well as on https://github.com/bitcoin/bitcoin/commit/453b4813ebc74859864803e9972b58e4be76a4d6~1, the [mempool variant tests](https://github.com/bitcoin/bitcoin/pull/
...

👍 BrandonOdiwuor approved a pull request: "rpc: Fix race in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/29262#pullrequestreview-1829703862)
Code Review ACK 5555d8db3313f893609eb0cf549bb597361d4466

💬 fanquake commented on pull request "depends: remove dependency on Darwin libtool":
(https://github.com/bitcoin/bitcoin/pull/29232#issuecomment-1898513919)
> Looks good.

I spoke too soon. There are multiple issues here.

The main one is differences in the upstream Autotools & CMake buildsystems that make master and this PR produce completely different output. One example, when building miniupnpc, its Autotools build sets `MINIUPNPC_GET_SRC_ADDR`, which means [`recvfrom`](https://www.man7.org/linux/man-pages/man2/recv.2.html) is used. However its CMake build does not, meaning `recv` is used instead. i.e:
```bash
# master
nm -C depends/aarch6
...

💬 vostrnad commented on issue "Timeout downloading block":
(https://github.com/bitcoin/bitcoin/issues/12291#issuecomment-1898517930)
Looking through my logs, I've only had two block download timeouts since upgrading to 26.0, and both happened while synchronizing after a period of downtime. They did still cause the synchronization to stall for ~10 minutes each though.

💬 instagibbs commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457506894)
yeah I was mostly hot-patching here, making sure it never returns too big(because it did very often)

💬 Sun0fABeach commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1898572308)
Of course there is always some other way of putting garbage on the chain, but looking at this problem through an exclusively technical lense will inevitably lead to nihilism.
Closing this particular vector sends a message about what type of activity is welcome, which influences the social layer, which in turn influences how much VC money gets pumped into these projects, exacerbating the problem.

🤔 murchandamus reviewed a pull request: "Add max_tx_weight to transaction funding options"
(https://github.com/bitcoin/bitcoin/pull/29264#pullrequestreview-1829800323)
Concept ACK, but instead of bailing immediately, it would be better to extend the conditions that immediately return a result to also require adhering to the max_weight.

💬 murchandamus commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1457513036)
Instead, how about:

```diff
- if (group.GetSelectionAmount() == nTargetValue) {
+ if (group.GetSelectionAmount() == nTargetValue && group.m_weight <= max_weight) {
```

👍 dergoegge approved a pull request: "build: Pass sanitize flags to instrument `libsecp256k1` code"
(https://github.com/bitcoin/bitcoin/pull/28875#pullrequestreview-1829825893)
reACK e39bae122c79b8dfaa5f7e02ff199dc8c2051d8a