💬 TheBlueMatt commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2223194931)
This seems like a lot of additional code that is very Sv2-specific. Is there no way to DRY a lot of this code up with connman itself? @pinheadmz may also have some thoughts here as I believe he's looking at replacing libevent with stuff that uses the common Core socket code anyway.

🤔 murchandamus reviewed a pull request: "#28984 package rbf followups"
(https://github.com/bitcoin/bitcoin/pull/30295#pullrequestreview-2172211559)
ACK 3f00aae14092ca076cff7f9ddf6155c601979fcd

Thanks for taking time to follow-up on my comments.

🤔 marcofleon reviewed a pull request: "fuzz: bound some miniscript operations to avoid fuzz timeouts"
(https://github.com/bitcoin/bitcoin/pull/30197#pullrequestreview-2172193524)
Lightly tested ACK 178a1da9c4da955c3dc78e6b03f110f687e82508. I tested on the three inputs brought up in https://github.com/bitcoin/bitcoin/issues/28812 and they all executed in 1ms. I can run the target for a while and see if any other timeouts come up. But looks good to me.

💬 marcofleon commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1674183937)
Based on the two inputs that caused the timeouts:
[clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13877884/clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt)
[clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt](https://github.com/bitcoin/bitcoin/files/15238132/clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt)

I think it's supposed to be counting the
...

💬 Sjors commented on pull request "Stratum v2 connman":
(https://github.com/bitcoin/bitcoin/pull/30332#issuecomment-2223234523)
> DRY a lot of this code up with connman itself?

Yes, I think the key is to make the copy-pasted parts of `CConnman::SocketHandlerConnected` reusable.

📝 maflcko opened a pull request: "rpc: Use CHECK_NONFATAL over Assert"
(https://github.com/bitcoin/bitcoin/pull/30429)
Any RPC method should not abort the whole node when an internal logic error happens.

Fix it by just aborting this single RPC method call when an error happens.

Also, fix the linter to find the fixed cases.

📝 fanquake converted_to_draft a pull request: "contrib: simplify `test-security-check`"
(https://github.com/bitcoin/bitcoin/pull/30423)
The current `test-security-check` script is hard to understand, and change (i.e https://github.com/bitcoin/bitcoin/pull/29987/files#diff-52aa0cda44721f089e53b128cb1232a876006ef257b211655456b17dfb2ec712); tests are also not done in isolation (when-possible). Fix that, and add missing checks. Simplifies future toolchain/security/hardening changes.

🚀 glozow merged a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596)

💬 mzumsande commented on issue "intermittent issue in feature_reindex.py: Reindex fails to continue after restart; `initload` thread hangs forever":
(https://github.com/bitcoin/bitcoin/issues/30424#issuecomment-2223311441)
> There is no reindex progess (it should pick up the previous work and try to make progess)

I don't think that is the problem here. We have the (in my opinion slightly weird) behavior that when reindex continues after an interrupted reindex run, it calls [ActivateBestChain](https://github.com/bitcoin/bitcoin/blob/9b480f7a25a737c9c4ebc33401e94d66c2da9ec3/src/validation.cpp#L5112-L5115) immediately when processing the genesis block, which result in multiple blocks being connected that were rein
...

💬 josibake commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2223334508)
> but rather in the same project so that Just Works is even an option

I don't agree that everything being in the same project is necessary for things to "Just Work." However, I think it's entirely premature to be talking about where the code lives, what project, etc until we've at least decided on a technical design.

> (as long as the interface is sufficiently performant - one very old complaint about getblocktemplate is we end up wasting multiple milliseconds encoding all the crap we sho
...

💬 hebasto commented on pull request "contrib: use c++ compiler rather than c compiler for binary checks":
(https://github.com/bitcoin/bitcoin/pull/30387#discussion_r1674313294)
```suggestion
std::printf("the quick brown fox jumps over the lazy god\\n");
```

Otherwise, it fails:
```
test1.cpp:5:21: warning: missing terminating " character
5 | std::printf("the quick brown fox jumps over the lazy god
| ^
test1.cpp:5:21: error: missing terminating " character
5 | std::printf("the quick brown fox jumps over the lazy god
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
test1.cpp:6:
...

⚠️ fanquake opened an issue: "ci: failure in `p2p_unrequested_blocks.py`"
(https://github.com/bitcoin/bitcoin/issues/30430)
Initially thought it was another occurance of #30390. https://github.com/bitcoin/bitcoin/actions/runs/9880260514/job/27288384486#step:27:584:
```bash
node0 2024-07-10T20:00:55.007544Z [msghand] [D:\a\bitcoin\bitcoin\src\validation.cpp:4305] [AcceptBlockHeader] [validation] AcceptBlockHeader: Consensus::ContextualCheckBlockHeader: 5539fb56d40fa94857d5bee8e1640a3d5a7e50fefa38d1361d2fd43110dcec03, time-too-old, block's timestamp is too early
node0 2024-07-10T20:00:55.007575Z [msghand] [D:\a\
...

💬 fanquake commented on issue "ci: failure in `wallet_multiwallet.py --legacy-wallet`":
(https://github.com/bitcoin/bitcoin/issues/29073#issuecomment-2223378634)
https://cirrus-ci.com/task/5564623879929856?logs=ci#L3392

⚠️ fanquake opened an issue: "ci: failure in `wallet_fundrawtransaction.py --legacy-wallet`"
(https://github.com/bitcoin/bitcoin/issues/30432)
https://cirrus-ci.com/task/5846098856640512?logs=ci#L3995
```bash
test 2024-07-11T14:51:16.715000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 158, in try_rpc
fun(*args, **kwds)
File "/ci_container_
...

💬 darosior commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1674330689)
It's `O(n)` but it's called in a loop over `n`.. So it effectively does `(n^2 - n)/2` operations, which is `O(n^2)`.

🚀 fanquake merged a pull request: "remove truc_policy from libbitcoin_common_a_SOURCES"
(https://github.com/bitcoin/bitcoin/pull/30427)

💬 stickies-v commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1674334965)
Ooh, right, because each character _is_ a wrapper. That also answers my [question](https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1668972900) about why we iterate in reverse, and why this implementation is correct.

This is probably fairly obvious to people familiar with Miniscript, but feel free to adopt this docstring anyway if it's helpful:

```cpp
auto count{0};
// A wrapper is a single character followed by a colon, e.g. `t:`.
// The colon is dropped between su
...

💬 stickies-v commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1674335378)
Resolved as per https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1674334965

👍 stickies-v approved a pull request: "fuzz: bound some miniscript operations to avoid fuzz timeouts"
(https://github.com/bitcoin/bitcoin/pull/30197#pullrequestreview-2172444820)
Light ACK 178a1da9c4da955c3dc78e6b03f110f687e82508

Code LGTM and approach seems reasonable, but there may be fuzzing and miniscript nuances I'm not considering.

💬 stickies-v commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1674340428)
I can't find any reference to fragments starting with '{' in the [BIP](https://github.com/bitcoin/bips/blob/master/bip-0379.md), is `|| ch == '{'` necessary?