🤔 janb84 reviewed a pull request: "bitcoin-cli: Add -ipcconnect option"
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2961926007)
ACK 9ffe57f81b2a6abd161ae010f07916b05e57191d
PR adds functionality to allow bitcoin-cli to connect to the node via IPC instead of TCP (if enabled) as part of the Multiprocess project
nit maybe fix grammar issues found by drahtbot https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2813015247 ?
- code review ✅
- build & tested ✅
- tested communication between cli and node binaries ✅
(https://github.com/bitcoin/bitcoin/pull/32297#pullrequestreview-2961926007)
ACK 9ffe57f81b2a6abd161ae010f07916b05e57191d
PR adds functionality to allow bitcoin-cli to connect to the node via IPC instead of TCP (if enabled) as part of the Multiprocess project
nit maybe fix grammar issues found by drahtbot https://github.com/bitcoin/bitcoin/pull/32297#issuecomment-2813015247 ?
- code review ✅
- build & tested ✅
- tested communication between cli and node binaries ✅
💬 janb84 commented on pull request "bitcoin-cli: Add -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2168951779)
```suggestion
std::unique_ptr<interfaces::Ipc> m_ipc;
```
```suggestion
private:
std::unique_ptr<interfaces::Ipc> m_ipc;
```
NIT can be made private ?
(https://github.com/bitcoin/bitcoin/pull/32297#discussion_r2168951779)
```suggestion
std::unique_ptr<interfaces::Ipc> m_ipc;
```
```suggestion
private:
std::unique_ptr<interfaces::Ipc> m_ipc;
```
NIT can be made private ?
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2168960313)
I didn't validate it, but I can. I think it is important that the new and old versions of `MemUsage` are roughly the same.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2168960313)
I didn't validate it, but I can. I think it is important that the new and old versions of `MemUsage` are roughly the same.
⚠️ sipa opened an issue: "`dumptxoutset` rollback feature does not take forks into account"
(https://github.com/bitcoin/bitcoin/issues/32817)
In the `dumptxoutset` RPC, when the rollback feature is used with a height H, it will issue an `InvalidateBlock` on the active-chain block at height H+1. However, if an inactive branch of the chain exists that forks off at height H or below, and extends to block H+1 or later (or has equivalent PoW to that), `InvalidateBlock` will reorg there, and dump will be for the tip of that branch, rather than the height-H ancestor of the main chain.
(https://github.com/bitcoin/bitcoin/issues/32817)
In the `dumptxoutset` RPC, when the rollback feature is used with a height H, it will issue an `InvalidateBlock` on the active-chain block at height H+1. However, if an inactive branch of the chain exists that forks off at height H or below, and extends to block H+1 or later (or has equivalent PoW to that), `InvalidateBlock` will reorg there, and dump will be for the tip of that branch, rather than the height-H ancestor of the main chain.
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2168974305)
Sweet, thanks for doing this! I was curious about any performance hits.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2168974305)
Sweet, thanks for doing this! I was curious about any performance hits.
💬 hebasto commented on pull request "Fix build on macOS when `qt@6` is installed":
(https://github.com/bitcoin/bitcoin/pull/32804#issuecomment-3008363545)
> Do we need this open? Rebased it's the same as #32814.
To make testing this branch easier?
(https://github.com/bitcoin/bitcoin/pull/32804#issuecomment-3008363545)
> Do we need this open? Rebased it's the same as #32814.
To make testing this branch easier?
💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169017125)
The symbols are "⌈⌉" ceiling function, can also be written as ceil(weight/4) fixed it in the PR decription for easy readablity
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169017125)
The symbols are "⌈⌉" ceiling function, can also be written as ceil(weight/4) fixed it in the PR decription for easy readablity
💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169033043)
Yes I agree. Removed.
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169033043)
Yes I agree. Removed.
💬 musaHaruna commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169035477)
Thanks for the review!!! I have added `[Feerates and vsize](feerates-and-vsize.md)` to the README.md to make the list exhaustive
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2169035477)
Thanks for the review!!! I have added `[Feerates and vsize](feerates-and-vsize.md)` to the README.md to make the list exhaustive
💬 fanquake commented on pull request "cmake: Explicitly specify `Boost_ROOT` for Homebrew's package":
(https://github.com/bitcoin/bitcoin/pull/32814#issuecomment-3008445000)
cc @purpleKarrot
(https://github.com/bitcoin/bitcoin/pull/32814#issuecomment-3008445000)
cc @purpleKarrot
💬 purpleKarrot commented on pull request "cmake: Explicitly specify `Boost_ROOT` for Homebrew's package":
(https://github.com/bitcoin/bitcoin/pull/32814#issuecomment-3008468460)
ACK
(https://github.com/bitcoin/bitcoin/pull/32814#issuecomment-3008468460)
ACK
🚀 fanquake merged a pull request: "depends: Bump boost to 1.88.0 and use new CMake buildsystem"
(https://github.com/bitcoin/bitcoin/pull/32665)
(https://github.com/bitcoin/bitcoin/pull/32665)
💬 fanquake commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3008481832)
#32665 has been merged.
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3008481832)
#32665 has been merged.
👋 hebasto's pull request is ready for review: "build: Find Boost in config mode"
(https://github.com/bitcoin/bitcoin/pull/32667)
(https://github.com/bitcoin/bitcoin/pull/32667)
💬 fanquake commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3008487317)
cc @purpleKarrot
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3008487317)
cc @purpleKarrot
💬 hebasto commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3008487737)
> #32665 has been merged.
Thanks! Rebased.
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3008487737)
> #32665 has been merged.
Thanks! Rebased.
💬 0xB10C commented on pull request "correct variable name in p2p_monitor.py":
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3008535601)
This just fixes a typo in AVALIABLE -> AVAILABLE. I don't think this is good use of reviewer time. Functionally is not affected.
(https://github.com/bitcoin/bitcoin/pull/32816#issuecomment-3008535601)
This just fixes a typo in AVALIABLE -> AVAILABLE. I don't think this is good use of reviewer time. Functionally is not affected.
💬 hebasto commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3008541105)
FWIW, I successfully built 639592548eeb4722c7655001d70a5f17bfda2a56 on Ubuntu 22.04 with the system-provided Boost 1.74.0.
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3008541105)
FWIW, I successfully built 639592548eeb4722c7655001d70a5f17bfda2a56 on Ubuntu 22.04 with the system-provided Boost 1.74.0.
💬 purpleKarrot commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3008541517)
ACK 639592548eeb4722c7655001d70a5f17bfda2a56
(https://github.com/bitcoin/bitcoin/pull/32667#issuecomment-3008541517)
ACK 639592548eeb4722c7655001d70a5f17bfda2a56
👍 fanquake approved a pull request: "cmake: Explicitly specify `Boost_ROOT` for Homebrew's package"
(https://github.com/bitcoin/bitcoin/pull/32814#pullrequestreview-2962244038)
ACK 8800b5acc1ef7abe6c5260ae0be5386b1d593a19 Checked that this plus #32805 fixes #31009
(https://github.com/bitcoin/bitcoin/pull/32814#pullrequestreview-2962244038)
ACK 8800b5acc1ef7abe6c5260ae0be5386b1d593a19 Checked that this plus #32805 fixes #31009