💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2237537095)
Rebased bdd68d5bca483071ca0729e6f0d2d71706ad21e7 -> fba04d7756c77ed9371c5dc90d3bebc9aa767f4b ([`pr/mine.6`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.6) -> [`pr/mine.7`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.7), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.6-rebase..pr/mine.7)) due to conflict with #30356

💬 brunoerg commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1683481047)
In 905e22b469a1e09df9ff0e98bf989a55642f301e "wallet: Remove IsMine from migration": Just a question, but could it be a vector of `CPubKey` then use `IsCompressed`?

🚀 achow101 merged a pull request: "rest: Reject negative outpoint index early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30444)

👍 stickies-v approved a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2185910679)
ACK c3a9c71c7077324bf0aa40f834f7537a14619340,

> are lacking a good description that actually says what the bug is.

I agree, I spent quite a bit of time getting to the bottom of this and more documentation would have sped that up

> but internally it is calling the uint256S function which expects a nul-terminated string

Maybe I'm being too pedantic (or wrong), but I'm not sure this is 100% correct? I don't think the `uint256S(std::string_view str)` `str` needs to be null-terminated. Ra
...

💬 stickies-v commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682892102)
review note: it seems the `while` loop was replaced with `for` loop purely for readability, I don't see any functional change here.

💬 hebasto commented on pull request "qa: Functional test improvements":
(https://github.com/bitcoin/bitcoin/pull/30463#discussion_r1683503930)
> Ok, so it can be reproduced in Linux with hard links.

Right. But we do not use hard links on non-Windows systems.

> However, this leads back to my original question: Why not change the other places as well?

In other places, `os.path.realpath(__file__)` is used in conjunction with relative paths in the `test/functional/` directory. Soft links (on non-Windows systems) and hard links (on Windows) are created for every file in the `test/functional/` directory recursively. Therefore, the c
...

💬 brunoerg commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1683511942)
In 905e22b469a1e09df9ff0e98bf989a55642f301e "wallet: Remove IsMine from migration": Maybe it's worth updating the documentation?

```
// For every script in mapScript, only the ISMINE_SPENDABLE ones are being tracked.
```

💬 achow101 commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#issuecomment-2237639016)
ACK 55b6d7be68a6f6c3882588ffd5b9349d885ed953

🚀 achow101 merged a pull request: "assumeutxo: Don't load a snapshot if it's not in the best header chain"
(https://github.com/bitcoin/bitcoin/pull/30320)

💬 achow101 commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#issuecomment-2237670866)
ACK 23333b7ed243071c9b4e4f04c727556d8065acbb

👍 hebasto approved a pull request: "depends: build FreeType with CMake"
(https://github.com/bitcoin/bitcoin/pull/29880#pullrequestreview-2186962592)
ACK ff4f3deb7b8adfcc90fb745440ce4be1176552ca, I've verified the actual compile options, they look sane.

🚀 achow101 merged a pull request: "net: Allow -proxy=[::1] on nodes with IPV6 lo only"
(https://github.com/bitcoin/bitcoin/pull/30245)

💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2237789775)
Rebased and since this is the last of the PRs addressing the TODOs comment in `feature_assumeutxo.py`, it now removes that whole section.

📝 theStack opened a pull request: "test: add creating/spending validity checks for rare output scripts"
(https://github.com/bitcoin/bitcoin/pull/30481)
This PR adds a functional test with a collection of output scripts that are "rare"/obscure, in the sense that they wouldn't be used or make sense within a regular payment transaction, but creating them is still considered standard, which is often considered surprising and counter-intuitive. They either
* use a weird pubkey encoding (-> hybrid pubkeys [in types P2PK, P2MS]),
* are provably unspendable (-> not-on-curve pubkeys [in types P2PK, P2MS, P2TR]), or
* are spendable by anyone with curr
...

💬 petertodd commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2237833642)
> @petertodd I'll give a non-TRUC version another try and report back. I may have been over-thinking things. If it works out it will be very similar in code structure...
>
> edit: oh of course I already noted in OP: We don't allow individual transactions with 0-fee outside of TRUC, for now due to trimming computation concerns. Cluster mempool does away with this restriction entirely. I'll rework the code to upgrade to non-TRUC usage once cluster mempool hits.

I think the worthwhile questio
...

💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683669595)
The other things can be safely set to false, so we can be sure they're false without an assertion. But if you've set a callback you've also gotten an `iterator` that you might intend to pass to `DeleteCallback` later, which would then crash if we had cleared `m_print_callbacks` here.

🤔 ryanofsky reviewed a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2187172873)
re: https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2185910679

> > but internally it is calling the uint256S function which expects a nul-terminated string
>
> Maybe I'm being too pedantic (or wrong), but I'm not sure this is 100% correct? I don't think the `uint256S(std::string_view str)` `str` needs to be null-terminated.

Yes, both explanations are saying the same thing but mine is unnecessarily confusing. The `uint256S` function "expects" a nul-terminated string becau
...

💬 ryanofsky commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1683689226)
re: https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1682892102

> review note: it seems the `while` loop was replaced with `for` loop purely for readability, I don't see any functional change here.

To be sure, replacing the previous while loop is an important part of the fix. If the previous while loop was kept, it could iterate past the end of the of `string_view` and read uninitialized memory. This for loop (and the while loop I suggested in https://github.com/bitcoin/bitcoin/pu
...

💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742290)
Added a `[...]` marker, since it was easy.

In order to trigger it you'd need to have 1MB of log message on a single, incomplete line, which seems a bit ridiculous. Long-term, I think it would make more sense to stop intentionally doing incomplete lines (since that introduces a race condition if other threads are also logging), and just add a newline if it's missing... *shrug*

💬 ajtowns commented on pull request "Early logging improvements":
(https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1683742446)
Changed to strprintf. Got a bit target fixated there...