💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1846404405)
435934fa020cec584b49c6151aa6b0d858054b37 oops, that should have been `&&`

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2482738819)
Pushed a bug fix, but still need to address the main feedback.

💬 maflcko commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2482760476)
Also, it would be good to add test coverage?

💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1846425672)
re https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834551086

Are you still working on this?

✅ maflcko closed a pull request: "nomerge: test malicious bidi char"
(https://github.com/bitcoin/bitcoin/pull/31314)

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2482785475)
I disabled ARM and macOS-cross as well, added a link to their failures to PR description TODO.

I think the `p2p_i2p_ports.py` failure in the previous releases job is spurious, so leaving that job in.

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2482789520)
@maflcko it's implicitly tested by the shutdown test, which is how I found this bug. Not sure how to explicitly test an early return?

💬 maflcko commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2482799390)
Are you sure it is covered, because the code is reported as uncovered in https://corecheck.dev/bitcoin/bitcoin/pulls/31297?

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2482804679)
@maflcko `feature_shutdown.py` only check that the early return is _not_ used. It makes sense to test the scenario where it is used too.

💬 maflcko commented on issue "Intermittent issue in p2p_i2p_ports.py AssertionError: [node 0] Expected messages "['Error connecting to [...].b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2482805416)
https://cirrus-ci.com/task/5063913392308224?logs=ci#L6167

💬 maflcko commented on pull request "test: Fix RANDOM_CTX_SEED use with parallel tests":
(https://github.com/bitcoin/bitcoin/pull/30737#issuecomment-2482830018)
For reference, the fuzz issue can be hit more frequent on machines that do not have nanosecond precision. GCE may not have it, according to the oss-fuzz crashes of the form:

```
libc++abi: terminating due to uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in remove_all: No such file or directory ["/tmp/test_common bitcoin/1731742732339168000/regtest/fuzzed_i2p_private_key"]
```

Ref: https://oss-fuzz.com/testcase-detail/5425475725361152

https:
...

💬 willcl-ark commented on pull request "Add `contrib/justfile` containing useful development workflow commands.":
(https://github.com/bitcoin/bitcoin/pull/31292#issuecomment-2482850113)
I really enjoy making justfiles for projects I work on.

And, whilst I would love to see some kind of community justfile added to this project, I can appreciate the hesitation to do so per the rationale given by @achow101 above.

I've therefore taken a different approach myself, hosting a justfile at [https://github.com/bitcoin-dev-tools/dotfiles](https://github.com/bitcoin-dev-tools/dotfiles/blob/master/justfile) which is designed to sit one directory higher than this repository's code, spe
...

🤔 rkrux reviewed a pull request: "wallet: Remove IsMine from migration code"
(https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2442077210)
Preliminary review fa155991ff30bc001229e1d71bf44e8782f02d17

I feel e6342ffdf0030f6350a07f325ce603fddfff1bd8 commit could be a separate PR, these functional tests are not dependent on the changes in the next two commits and don't need to be tied to these changes.

💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846226163)
`Lambda`

💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846392613)
```diff
- { return key.size() == 33; });
+ { return key.size() == CPubKey::COMPRESSED_SIZE; });
```

There's another usage down below as well on line 1816: https://github.com/bitcoin/bitcoin/commit/fa155991ff30bc001229e1d71bf44e8782f02d17#diff-55ce403d9c3440b4a38a261b7d452f84dd9a1d09b2a5ef1b4d3ac0c3567b9106R1816

💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846291918)
We can't reuse `IsMineSigVersion` because it will be removed soon?
Can we add few comments here like done in `IsMineSigVersion`? https://github.com/bitcoin/bitcoin/blob/28.x/src/wallet/scriptpubkeyman.cpp#L56-L67

💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846279325)
`arbitrary`

💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846396195)
```diff
- std::vector<std::vector<unsigned char>> sols;
+ std::vector<std::vector<unsigned char>> solutions;
```

Agree, would be easier to read.

💬 rkrux commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1846447833)
There must be a reason for making `IsCompressedPublicKey()` here static, which I dont know about, otherwise, it could be used within `all_of` here, thereby getting rid of some logic duplication: https://github.com/bitcoin/bitcoin/blob/28.x/src/script/interpreter.cpp#L85

💬 rkrux commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1846482350)
I don't quite understand this^.

From what I understood:
If `include_mempool` is true, then the `blockhash` can be empty if the transaction is in mempool.
If `include_mempool` is false, then the `blockhash` can't be empty because there is no other way for the transaction to appear in the result here besides being in a block.

Am I missing something?