💬 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
...
💬 maflcko commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2542640064)
```suggestion
height = self.nodes[0].getblockcount() + 1
block_time = self.nodes[0].getblock(tip)['time'] + 1
invalid_blocks = []
for i in range(7): # invalid chain must be at least 6 blocks longer to trigger warning
block = create_block(int(tip, 16), create_coinbase(height), block_time)
```
small nit: Could ensure time and height are incremented in the same places?
(https://github.com/bitcoin/bitcoin/pull/33893#discussion_r2542640064)
```suggestion
height = self.nodes[0].getblockcount() + 1
block_time = self.nodes[0].getblock(tip)['time'] + 1
invalid_blocks = []
for i in range(7): # invalid chain must be at least 6 blocks longer to trigger warning
block = create_block(int(tip, 16), create_coinbase(height), block_time)
```
small nit: Could ensure time and height are incremented in the same places?
💬 Eunovo commented on pull request "wallet/rpc: fix listdescriptors RPC fails to return descriptors with private key information when wallet contains descriptors missing any key":
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2542654939)
I added comments where `any_success` was used.
(https://github.com/bitcoin/bitcoin/pull/32471#discussion_r2542654939)
I added comments where `any_success` was used.
💬 pinheadmz commented on pull request "Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3553506888)
Push to 8c81bdf2532aa1c61e25d367a480ce3aa71362ca:
- Rebase on #32747 which has gotten lots of good feedback, and is also rebased on master
- Fix four nits from @vasild
This push is mainly for a CI sanity check, then I'm going to put it in draft while I refactor for @theuni main feedback which will be removing the sockman abstraction and implementing the I/O loop directly in `HTTPServer`
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-3553506888)
Push to 8c81bdf2532aa1c61e25d367a480ce3aa71362ca:
- Rebase on #32747 which has gotten lots of good feedback, and is also rebased on master
- Fix four nits from @vasild
This push is mainly for a CI sanity check, then I'm going to put it in draft while I refactor for @theuni main feedback which will be removing the sockman abstraction and implementing the I/O loop directly in `HTTPServer`