💬 ryanofsky commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2542202272)
re: https://github.com/bitcoin/bitcoin/pull/33770#discussion_r2529118333

> nit: `strprintf` isn't needed anymore.

Thanks, removed

💬 fjahr commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3552979597)
> > Even though @ryanofsky didn't explicitly review this PR here yet my understanding is that he didn't intend #33770 as a replacement of this either, but please correct me if I am wrong.
>
> I did intend to #33770 to be an alternative this PR and as far I I know there wouldn't be any benefits to this PR if #33770 is merged, unless I'm missing something. Both this PR and #33770 are addressing the problem of `-asmap` default value not matching its default behavior. #33770 addresses that by mak
...

💬 brunoerg commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3552990665)
reACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4

Verified it removed the `strprintf` and `(default: none)`.

💬 fjahr commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3552996786)
reACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4

📝 fanquake converted_to_draft a pull request: "subprocess: Let shell parse command on non-Windows systems"
(https://github.com/bitcoin/bitcoin/pull/32577)
Fixes https://github.com/bitcoin/bitcoin/issues/32574.

The `subprocess::Popen` constructor has two overloads: https://github.com/bitcoin/bitcoin/blob/7763e86afa045910a14ac9b2cd644927a447370b/src/util/subprocess.h#L938-L941

During the migration from Boost.Process in https://github.com/bitcoin/bitcoin/pull/28981, the second was chosen for two reasons: (1) it minimized changes at the call sites, and (2) it [addressed](https://github.com/bitcoin/bitcoin/pull/28981#issuecomment-1837238921) quot
...

💬 maflcko commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542278811)
> For example, suppose someone created a wallet where -named is enabled by default using: `./bitcoin-cli createwallet "my=wallet"` internally this will be passed as a -named argument and the user will get the error about "Unknown named parameter my" even though the user passed the argument by position. That's why I think your suggested approach might not work here, but you can clarify that if you think so or if I'm missing any point.

I don't understand why my suggestion does not work. `my=` i
...

💬 brunoerg commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2542333645)
8a40e10c1ef4cfdd5fe19796131af2b5a34531ce: nit: `i` is not used.
```suggestion
for _ in range(7): # invalid chain must be at least 6 blocks longer to trigger warning
```

💬 exp3rimenter commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#issuecomment-3553135639)
Concept ACK on refactoring bool to return state.

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2542387292)
Ah, rebase fail I guess, done.

💬 ryanofsky commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3553263582)
Thanks for explaining. Now I see why you still want this PR. In the short term with #33770 there are no problems, and in the long terms with embedded asmap turned on by default there are no problems. But in the medium turn with embedded asmap data present but not turned on by default, you dislike the fact that `-asmap=1` would load a file called `1` if we don't take additional steps to prevent this.

To clarify:

- In the short term, with [#33770](https://github.com/bitcoin/bitcoin/pull/3377
...

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2542466280)
Ok, I hope I got them all now.

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2542466628)
Done. And I just clarified that failure means 0 in python, otherwise there would have been more edits necessary to make this clear, for example the line below your suggestion still talks about a failure too.

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2542467175)
Done

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2542467832)
Fixed

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2542468167)
Indeed, removed.

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2542468478)
That works, done.

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2542486234)
Renamed

💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3553314078)
Rebased since #33026 was merged 🥳 and addressed all comments. Thanks for the reviews!

💬 stratospher commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2542548870)
oh good point.

💬 stratospher commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2542550696)
done. I've made it an error instead so that the user doesn't accidentally switch off listening mode.