💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637053225)
> I think all those benefits could be achieved by adding the glue yet having the developer who wants to test checkout the upstream repo into a subdirectory manually to use it (cmake can complain clearly if missing)

@luke-jr, Yes, we could provide a glue script to download the git repository instead of mirroring it in a git subtree, pinning it to a hash to avoid bringing in unreviewed changes. This is essentially what depends system does now. Do you think a glue script would be better than a s
...

👍 instagibbs approved a pull request: "test: make sure we are on sync with a peer before checking if they have sent a message"
(https://github.com/bitcoin/bitcoin/pull/31760#pullrequestreview-2596001593)
ACK 3f4b104b1b7e1b87c0be8e395e02b6ae3c5d7b08

Makes sense and can't really hurt to apply it every time.

💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1943081201)
> it returned the same value or very close each time. I did not see any 100% values before reaching the tip.

It makes sense bc your node is not isolated, but when you turn it on before having any peer I think it returns 1.0 and then it goes back to 0.x when it receives some header.

🤔 rkrux reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2595302398)
Build and migration tests successful at d5e28457a099cd546e757984043f28ba9f6689b1.

Utilising `isMine()` instead of reverse engineering looks to me a cautious approach to take, Concept ACK.
Great PR and enough context to go through for which I will review the PR again.

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942646524)
`legacy`

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942794684)
`// Invalid scripts such as P2SH-P2SH and P2WSH-P2SH, among others, will be added as candidates.`

In the below function, I notice script itself, P2SH, P2WSH, P2SH-P2WSH being added only. These invalid scripts could be added as candidates because the underlying `script` could be a P2SH?

With this script nesting involved, it makes me want to read the logic of `IsMine` to see how it handles it!

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942786866)
`contain`

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942841714)
Nit:

```diff
- if (!Assume(spks.size() == 0)) {
+ if (!Assume(spks.empty())) {
```

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942897067)
`have`

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942865073)
Thanks for this comment, very helpful!

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942990827)
`self.old_node`?

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1943021410)
Where was the first check?

💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1943008904)
Nit: Can add a corresponding call before sending the transaction that checks the balance is 6.

🤔 sipa reviewed a pull request: "tracing: network connection tracepoints"
(https://github.com/bitcoin/bitcoin/pull/25832#pullrequestreview-2596026595)
utACK e3622a969293feea75cfadc8f7c6083edcd6d8de

💬 sipa commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1943088145)
It's not very nice to expose this internal value in the interface, if we want it to be stable. What about using the [BIP155 network ID](https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki#user-content-Specification) instead?

📝 hebasto opened a pull request: "depends: Avoid using the `-ffile-prefix-map` compiler option"
(https://github.com/bitcoin/bitcoin/pull/31800)
This PR is similar to https://github.com/bitcoin/bitcoin/pull/31337 and applies analogous changes to all dependency packages.

The issue was [recently noticed](https://github.com/bitcoin/bitcoin/pull/31661#discussion_r1923896475) when `-ffile-prefix-map` was added to the `libevent` package, which is built in OSS-Fuzz.

This PR replaces `-ffile-prefix-map` from in packages for consistency.

💬 hebasto commented on issue "fuzz: oss-fuzz coverage build is failing":
(https://github.com/bitcoin/bitcoin/issues/31770#issuecomment-2637098504)
A fix has been proposed in https://github.com/bitcoin/bitcoin/pull/31800.

💬 instagibbs commented on pull request "test: fixes p2p_ibd_txrelay wait time":
(https://github.com/bitcoin/bitcoin/pull/31759#issuecomment-2637099528)
meta: do we ever want a test to bumpmocktime without setting it first?

💬 luke-jr commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637110634)
Yes, I think it would be better.

I would just have the git clone command documented though, no need to automate it?

💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1943113886)
> Maybe that's how it is used at the moment, but the documentation of the function implies that the estimate can be used on any block, not that pindex necessarily is the latest block.

As long as we have good headers using any block should not be a problem and should return a reasonable estimate.


> That seems bad to me. Being isolated from the network shouldn't make it report values _that_ far off! The old current-time-based estimate might have edge cases around 1, but apart from that it
...