💬 theStack commented on pull request "Introduce field element and group element classes to test_framework/key.py":
(https://github.com/bitcoin/bitcoin/pull/26222#discussion_r1154452161)
That's a typo I guess?
```suggestion
raising the argument to the power (p + 1) / 4.
```

👋 TheCharlatan's pull request is ready for review: "refactor (tidy): Fixes after enable-debug configure"
(https://github.com/bitcoin/bitcoin/pull/27353)

💬 TheCharlatan commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1491914824)
> I think this PR diff is an improvement.

I'll undraft this then, even if I still don't understand why `clang-tidy` seems to catch more with debug enabled.

💬 glozow commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1491916125)
> > Besides, I'm also not sold that this lint is an actual improvement.
>
> I think this PR diff _is_ an improvement.

Could you explain why it's an improvement? It's unclear to me why const references to iterators would be an improvement, I was under the impression that iters by value were fine.

💬 furszy commented on issue "wallet: Data race in GetOrCreateLegacyScriptPubKeyMan vs IsMine":
(https://github.com/bitcoin/bitcoin/issues/27354#issuecomment-1491916385)
> However, I think this may also happen in production, so a proper fix would be to protect m_spk_managers with a mutex? If not, it might be sufficient to add a SyncWithValidationInterfaceQueue() somewhere in the test.

It is hard for this race to occur in production due to the wallet registering to the validation interface after loading, which means that the legacy spkm exists before any chain event takes place. However, other races may still occur. Furthermore, the spkm maps are guarded by `c
...

💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1491935771)
> https://github.com/bitcoin/bitcoin/blob/328087d16f4362a23a72575fee930a275efbf3dd/src/net.cpp#L1890
>
>
> to determine the `fCountFailure` bool, which tells addrman to possibly deprioritize selecting an address after various failed attempts (`nAttempts`). As far as I can see, this logic should equally be applied to addrs from alt networks, so it might be better to leave `setConnected` unchanged as sdaftuar suggested?

@lightlike Good observation, I missed this. I think that bit of code
...

📝 darosior opened a pull request: "miniscript: explicit cast instead of comparing integers of different signs"
(https://github.com/bitcoin/bitcoin/pull/27382)
Fixes #27381

💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1154508362)
I think we should generally avoid adding `assert()` to networking code, unless we're sure that it's really better to crash than to continue, eg because the node is in an inconsistent or corrupt state and is unable to continue running. That doesn't seem to be the case here, so I think we could use `Assume(false)` instead, so that we only get crashes in debug builds and our CI environment.

💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1154506161)
As I mention in my comment [elsewhere](https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1491935771), I think we can drop this logic.

👍 furszy approved a pull request: "refactor: remove unused param from legacy pubkey interface"
(https://github.com/bitcoin/bitcoin/pull/27274)
ACK 1869310f3cfa4ab26b5090d8a4002eefdc84870e

💬 MarcoFalke commented on issue "Feature request: alert PR author in case of CI failure":
(https://github.com/bitcoin/bitcoin/issues/27178#issuecomment-1492006399)
Can I also get check runs? Looks like no events are sent out for check suites on a re-run.

💬 pinheadmz commented on issue "Manual-pruning cursor rewind":
(https://github.com/bitcoin/bitcoin/issues/19807#issuecomment-1492010672)
> I no longer personally need this feature, but I'm simply describing the need as I understand it should people want to be using manual pruning in the future.

Ok thanks for engaging me anyway. I still don't see why anyone would want this feature except for re-scanning an old wallet on a pruned node. If you can provide a reasonable use case we can keep this issue open, but otherwise I think we should close and follow #21267

> a node should just serve the blocks it has.

Actually pruned no
...

💬 pinheadmz commented on issue "Export a watch wallet only (with descriptors and without private keys) for an air gap setup":
(https://github.com/bitcoin/bitcoin/issues/24829#issuecomment-1492017381)
Hm, seems to work for me on master branch. The taproot addresses are identical from each wallet.


💬 pinheadmz commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1154561718)
Two little questions about this:
- Why does this solve the immature coinbase spending?
- Does it make sense to just squash into the previous line? i.e. `COINBASE_MATURITY + 35` if those 35 blocks even needed anymore

💬 pinheadmz commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1154562830)
It's always nice to have options - but since none of the tests actually need to set this to `False` anymore, does it make sense to remove the argument and just filter out immature coins all the time no matter what anyway?

💬 theStack commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#discussion_r1154547880)
fell-free-to-ignore tiny-nit: could just do it in one call
```suggestion
self.generate(self.wallet, 35 + COINBASE_MATURITY)
```

👍 theStack approved a pull request: "test: fix intermittent issue in `feature_bip68_sequence`"
(https://github.com/bitcoin/bitcoin/pull/27177)
ACK f8ebcb823fc5f39adc87ea6cf079f3e72ee44f30

(Didn't run interface_usdt_mempool.py locally, as I'm currently not having a build with tracepoints available, but the changes look reasonable.)

💬 hebasto commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1492035512)
> I'll undraft this then, even if I still don't understand why `clang-tidy` seems to catch more with debug enabled.

I suspect the `-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE` definition.

💬 hebasto commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1492042782)
> > > Besides, I'm also not sold that this lint is an actual improvement.
> >
> >
> > I think this PR diff _is_ an improvement.
>
> Could you explain why it's an improvement? It's unclear to me why const references to iterators would be an improvement, I was under the impression that iters by value were fine.

Well, my main point was enforcing const-corectness, but I _missed_ the fact that `CTxMemPool::txiter` _is_ a `const_iterator`.

Making a constness at the call site obvious for
...

💬 stickies-v commented on pull request "miniscript: explicit cast instead of comparing integers of different signs":
(https://github.com/bitcoin/bitcoin/pull/27382#discussion_r1154600109)
Maybe best to avoid C-style casts?

```suggestion
return static_cast<uint32_t>(std::count(subs.begin(), subs.end(), true)) >= node.k;
```