💬 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.
(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
...
(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.
(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.
(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
(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
(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.
(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.
(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
(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!
(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.
(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.
(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.
💬 stratospher commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2542555935)
> Is there any reason to not have DisableV10nClearnet do the GetNetClass internally?
done. the initial version in the PR with both outbound+ inbound v2 only required another function to detect inbound onion and we had to pass network. but we can remove it for outbound v2 only.
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2542555935)
> Is there any reason to not have DisableV10nClearnet do the GetNetClass internally?
done. the initial version in the PR with both outbound+ inbound v2 only required another function to detect inbound onion and we had to pass network. but we can remove it for outbound v2 only.
💬 vostrnad commented on pull request "init: Require explicit -asmap filename":
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3553386556)
utACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4
(https://github.com/bitcoin/bitcoin/pull/33770#issuecomment-3553386556)
utACK 288b8c30be42f2879d1a1f3d5ec3cac13f87ace4
💬 stratospher commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2542560527)
makes sense. changed it to `RequiresV2ForOutbound`. hopefully it's better.
(https://github.com/bitcoin/bitcoin/pull/30951#discussion_r2542560527)
makes sense. changed it to `RequiresV2ForOutbound`. hopefully it's better.
💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542570737)
> I don't understand why my suggestion does not work. `my=` is not a valid named argument for `createwallet`, so the check `"my=wallet".starts_with("wallet_name=")` evaluates to false for all args, and the argument is correctly identified as a positional arg. Conversely, if you were passing `wallet_name=bla`, the check would also correctly identify the named arg.
That's what something similar to the current code is doing internally but with more checks and conditions. So one of the reasons yo
...
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542570737)
> I don't understand why my suggestion does not work. `my=` is not a valid named argument for `createwallet`, so the check `"my=wallet".starts_with("wallet_name=")` evaluates to false for all args, and the argument is correctly identified as a positional arg. Conversely, if you were passing `wallet_name=bla`, the check would also correctly identify the named arg.
That's what something similar to the current code is doing internally but with more checks and conditions. So one of the reasons yo
...
💬 plebhash commented on issue "Block template memory management (for IPC clients)":
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3553407353)
we're discussing this on a Sv2 call and @GitGab19 pointed out that whatever default value is set for `-maxtemplatepoolsize` should be a reasonable number
the main concern is having values that are too low, which would make templates be discarded too soon
we're not sure what the ideal value should be, but just writing this to bring awareness to this
(https://github.com/bitcoin/bitcoin/issues/33899#issuecomment-3553407353)
we're discussing this on a Sv2 call and @GitGab19 pointed out that whatever default value is set for `-maxtemplatepoolsize` should be a reasonable number
the main concern is having values that are too low, which would make templates be discarded too soon
we're not sure what the ideal value should be, but just writing this to bring awareness to this
💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542581787)
re: https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542278811
Thanks for the reply. I think you have all the facts right, and I accept all the points you are making, even if we might prefer different tradeoffs & possible solutions. My point was just that `RPCConvertNamedValues` was using a poor heuristic before this PR, and now it is using a better one, without requiring every parameter to be listed in the table. If you do want to list every parameter in the table and simplify the
...
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542581787)
re: https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2542278811
Thanks for the reply. I think you have all the facts right, and I accept all the points you are making, even if we might prefer different tradeoffs & possible solutions. My point was just that `RPCConvertNamedValues` was using a poor heuristic before this PR, and now it is using a better one, without requiring every parameter to be listed in the table. If you do want to list every parameter in the table and simplify the
...
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2542621198)
My bad, I must have tested a slightly different comparator. The suggested ones are deterministic.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2542621198)
My bad, I must have tested a slightly different comparator. The suggested ones are deterministic.
👍 maflcko approved a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3483548675)
review ACK 8a40e10c1ef4cfdd5fe19796131af2b5a34531ce 🌃
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 8a40e10c1ef4cfdd5fe19
...
(https://github.com/bitcoin/bitcoin/pull/33893#pullrequestreview-3483548675)
review ACK 8a40e10c1ef4cfdd5fe19796131af2b5a34531ce 🌃
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 8a40e10c1ef4cfdd5fe19
...