💬 maflcko commented on pull request "AssumeUTXO follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28562#discussion_r1386889955)
Another report, which could replicate the crash, see #https://github.com/bitcoin/bitcoin/pull/28791
(https://github.com/bitcoin/bitcoin/pull/28562#discussion_r1386889955)
Another report, which could replicate the crash, see #https://github.com/bitcoin/bitcoin/pull/28791
💬 luke-jr commented on pull request "contrib: Add asmap-tool":
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1802243288)
Concept NACK
>The motivation is that we should maintain the tooling for de- and encoding asmap files within the bitcoin core repository because it is not possible to use an asmap file that is not encoded.
That's not a reason. Ideally, the current repo should be split into several itself - but we have technical hurdles to get to that point. No reason to add more here "just because"
(https://github.com/bitcoin/bitcoin/pull/28793#issuecomment-1802243288)
Concept NACK
>The motivation is that we should maintain the tooling for de- and encoding asmap files within the bitcoin core repository because it is not possible to use an asmap file that is not encoded.
That's not a reason. Ideally, the current repo should be split into several itself - but we have technical hurdles to get to that point. No reason to add more here "just because"
💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1802246176)
@naumenkogs If the user has *also* set a low `maxconnections` value then [`SelectNodeToEvict()`](https://github.com/bitcoin/bitcoin/blob/19d1ba1b4141f744cca291ff2d99d0b8ffcf946d/src/node/eviction.cpp#L186-L197) might return null.
Quick math 4+8+4+8+4 = 28 protected nodes. Plus 8 full outbound and 2 block only = 38. So if a user runs a full node on limited hardware (like my Raspberry Pi at home) and have something like `-maxconnections=30` then even with the existing `whitebind` permissions, t
...
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1802246176)
@naumenkogs If the user has *also* set a low `maxconnections` value then [`SelectNodeToEvict()`](https://github.com/bitcoin/bitcoin/blob/19d1ba1b4141f744cca291ff2d99d0b8ffcf946d/src/node/eviction.cpp#L186-L197) might return null.
Quick math 4+8+4+8+4 = 28 protected nodes. Plus 8 full outbound and 2 block only = 38. So if a user runs a full node on limited hardware (like my Raspberry Pi at home) and have something like `-maxconnections=30` then even with the existing `whitebind` permissions, t
...
🤔 luke-jr requested changes to a pull request: "rpc: Add script verification flags to getdeploymentinfo"
(https://github.com/bitcoin/bitcoin/pull/28806#pullrequestreview-1720807012)
Getting some deja vu here - is there another PR for this already?
(https://github.com/bitcoin/bitcoin/pull/28806#pullrequestreview-1720807012)
Getting some deja vu here - is there another PR for this already?
💬 luke-jr commented on pull request "rpc: Add script verification flags to getdeploymentinfo":
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1386914067)
Maybe number form should also be available for users of libbitcoinconsensus?
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1386914067)
Maybe number form should also be available for users of libbitcoinconsensus?
💬 luke-jr commented on pull request "rpc: Add script verification flags to getdeploymentinfo":
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1386911888)
If we're exposing this, probably better to call it "BIP16" since p2wsh and p2tr(sometimes) are also p2sh.
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1386911888)
If we're exposing this, probably better to call it "BIP16" since p2wsh and p2tr(sometimes) are also p2sh.
💬 luke-jr commented on pull request "rpc: Add script verification flags to getdeploymentinfo":
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1386913949)
This should not be a comma-deliminated string, but an array of strings.
(https://github.com/bitcoin/bitcoin/pull/28806#discussion_r1386913949)
This should not be a comma-deliminated string, but an array of strings.
💬 luke-jr commented on pull request "rpc: Add script verification flags to getdeploymentinfo":
(https://github.com/bitcoin/bitcoin/pull/28806#issuecomment-1802267067)
Ah, #10730
(https://github.com/bitcoin/bitcoin/pull/28806#issuecomment-1802267067)
Ah, #10730
📝 instagibbs opened a pull request: "fuzz: Minor improvements to tx_package_eval target"
(https://github.com/bitcoin/bitcoin/pull/28825)
Exercises `DIFFERENT_WITNESS` and attempts to make invalid/duplicate inputs.
(https://github.com/bitcoin/bitcoin/pull/28825)
Exercises `DIFFERENT_WITNESS` and attempts to make invalid/duplicate inputs.
👍 hebasto approved a pull request: "ci: remove note re M1 usage"
(https://github.com/bitcoin/bitcoin/pull/28823#pullrequestreview-1720929884)
> M1 is now available in GitHub CI, but we don't currently have a plan to use it...
... for the only reason that it is not free for now. I believe, we still have a plan to use a free M1 runner in the future.
> ... so remove the comment.
Yes. It is outdated and misleading with the current wording.
ACK 8cbb6196913b22006dac75f719a2834ab0d6c94f.
(https://github.com/bitcoin/bitcoin/pull/28823#pullrequestreview-1720929884)
> M1 is now available in GitHub CI, but we don't currently have a plan to use it...
... for the only reason that it is not free for now. I believe, we still have a plan to use a free M1 runner in the future.
> ... so remove the comment.
Yes. It is outdated and misleading with the current wording.
ACK 8cbb6196913b22006dac75f719a2834ab0d6c94f.
💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1386940970)
great catch thanks, will fix.
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1386940970)
great catch thanks, will fix.
💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1365894733)
thanks, removed
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1365894733)
thanks, removed
🤔 pinheadmz reviewed a pull request: "net: support unix domain sockets for -proxy and -onion"
(https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1688303909)
force pushed to clean up git commit history. diff is null
(https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1688303909)
force pushed to clean up git commit history. diff is null
💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1386940797)
Good observation. This is how `ConnectSocketDirectly()` and `ConnectThroughProxy()` are implemented on master currently as well. I think the reason is that we actually send the SOCKS5 proxy the destination host as a string whereas for the direct connection we actually get a socket from the OS. So even if throughProxy accepted a CService, we'd have to split into host string and port right away, anyway. I think I'll leave this alone for now unless other reviewers want to weigh in.
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1386940797)
Good observation. This is how `ConnectSocketDirectly()` and `ConnectThroughProxy()` are implemented on master currently as well. I think the reason is that we actually send the SOCKS5 proxy the destination host as a string whereas for the direct connection we actually get a socket from the OS. So even if throughProxy accepted a CService, we'd have to split into host string and port right away, anyway. I think I'll leave this alone for now unless other reviewers want to weigh in.
💬 achow101 commented on pull request "ci: remove note re M1 usage":
(https://github.com/bitcoin/bitcoin/pull/28823#issuecomment-1802419915)
ACK 8cbb6196913b22006dac75f719a2834ab0d6c94f
(https://github.com/bitcoin/bitcoin/pull/28823#issuecomment-1802419915)
ACK 8cbb6196913b22006dac75f719a2834ab0d6c94f
🚀 achow101 merged a pull request: "ci: remove note re M1 usage"
(https://github.com/bitcoin/bitcoin/pull/28823)
(https://github.com/bitcoin/bitcoin/pull/28823)
💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1802426556)
Updated 995aa6b9cb5d1ea685a5d2ac6767d21b373e4c84 -> 3e7595b11bdad260efb39adc42677ed0beae186d ([simplifyMemPoolInteractions_10](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_10) -> [simplifyMemPoolInteractions_11](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_10..simplifyMemPoolInteractions_11))
* Addressed @ismaelsadeeq's [comment](https://github.co
...
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1802426556)
Updated 995aa6b9cb5d1ea685a5d2ac6767d21b373e4c84 -> 3e7595b11bdad260efb39adc42677ed0beae186d ([simplifyMemPoolInteractions_10](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_10) -> [simplifyMemPoolInteractions_11](https://github.com/TheCharlatan/bitcoin/tree/simplifyMemPoolInteractions_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/simplifyMemPoolInteractions_10..simplifyMemPoolInteractions_11))
* Addressed @ismaelsadeeq's [comment](https://github.co
...
🤔 glozow reviewed a pull request: "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access"
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1721061557)
ACK 3e7595b11bdad260efb39adc42677ed0beae186d
(https://github.com/bitcoin/bitcoin/pull/28391#pullrequestreview-1721061557)
ACK 3e7595b11bdad260efb39adc42677ed0beae186d
💬 glozow commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387067811)
nit 65c4b3bac0894dfdf7e1c756697f9dd0da1f97e7: don't need to go to uint256 and back again
```suggestion
const Txid& hash = txinfo.tx->GetHash();
const auto entry{pool.GetEntry(hash)};
```
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1387067811)
nit 65c4b3bac0894dfdf7e1c756697f9dd0da1f97e7: don't need to go to uint256 and back again
```suggestion
const Txid& hash = txinfo.tx->GetHash();
const auto entry{pool.GetEntry(hash)};
```
📝 hebasto opened a pull request: "ci: Switch IWYU to `clang_17` branch"
(https://github.com/bitcoin/bitcoin/pull/28826)
The IWYU version [0.21](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.21) has been tagged, and the `clang_17` branch is available now.
(https://github.com/bitcoin/bitcoin/pull/28826)
The IWYU version [0.21](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.21) has been tagged, and the `clang_17` branch is available now.