💬 instagibbs commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2359361516)
fdcb5f17bd100eb2c1765c13bbb8367d03fc7ea5
micro-nit for readability
```Suggestion
MakeAcceptable(*cluster, level);
```
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2359361516)
fdcb5f17bd100eb2c1765c13bbb8367d03fc7ea5
micro-nit for readability
```Suggestion
MakeAcceptable(*cluster, level);
```
💬 marcofleon commented on pull request "fuzz: reduce iterations in slow targets":
(https://github.com/bitcoin/bitcoin/pull/33429#issuecomment-3308577557)
There should be no loss of coverage when running on the inputs in qa-assets.
For `tx_pool_standard` I saw a speed up from 1068 to 720 seconds with this change. I think the loop in `initialize_tx_pool` is still slowing it down? I would need to look into some more, but I left that as is for now.
(https://github.com/bitcoin/bitcoin/pull/33429#issuecomment-3308577557)
There should be no loss of coverage when running on the inputs in qa-assets.
For `tx_pool_standard` I saw a speed up from 1068 to 720 seconds with this change. I think the loop in `initialize_tx_pool` is still slowing it down? I would need to look into some more, but I left that as is for now.
💬 Sjors commented on pull request "Update libmultiprocess subtree to fix intermittent mptest hang":
(https://github.com/bitcoin/bitcoin/pull/33412#issuecomment-3308650255)
ACK c49a43591f88dc199cc04e76f3cfcb7ba136f1a6
(https://github.com/bitcoin/bitcoin/pull/33412#issuecomment-3308650255)
ACK c49a43591f88dc199cc04e76f3cfcb7ba136f1a6
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360439278)
Fixed name
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360439278)
Fixed name
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360439282)
No problem, I put them back to the beginning
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360439282)
No problem, I put them back to the beginning
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360439251)
I think this is trivial to review.
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360439251)
I think this is trivial to review.
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360439503)
This seemed like the time to simplify these, but I don't mind, reverted.
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360439503)
This seemed like the time to simplify these, but I don't mind, reverted.
💬 l0rinc commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360440096)
I did this deliberately, but thinking a bit more I think we can make it work. Pushed something similar, let me know what you think.
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2360440096)
I did this deliberately, but thinking a bit more I think we can make it work. Pushed something similar, let me know what you think.
👍 darosior approved a pull request: "p2p: Increase tx relay rate"
(https://github.com/bitcoin/bitcoin/pull/28592#pullrequestreview-3241205204)
utACK b81f37031c8f2ccad9346f1b65ee0f8083c44796. A value lower than 14 makes more sense to me, but 14 is an improvement on today's 7.
[Mainnet.observer](https://mainnet.observer/charts/transactions-per-day/) has historical statistics that can help guide our decision. Back when the 7 tx/s was picked, there was around 2.3 tx/s (when averaged over days). In this regard, doubling the default now that there is around 4.6 tx/s makes sense. However i think a lower value than 14 tx/s would be more ap
...
(https://github.com/bitcoin/bitcoin/pull/28592#pullrequestreview-3241205204)
utACK b81f37031c8f2ccad9346f1b65ee0f8083c44796. A value lower than 14 makes more sense to me, but 14 is an improvement on today's 7.
[Mainnet.observer](https://mainnet.observer/charts/transactions-per-day/) has historical statistics that can help guide our decision. Back when the 7 tx/s was picked, there was around 2.3 tx/s (when averaged over days). In this regard, doubling the default now that there is around 4.6 tx/s makes sense. However i think a lower value than 14 tx/s would be more ap
...
💬 achow101 commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33416#issuecomment-3308904174)
Should also backport #31407
(https://github.com/bitcoin/bitcoin/pull/33416#issuecomment-3308904174)
Should also backport #31407
💬 l0rinc commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2360633853)
I still think this should need a test as well, otherwise it just seems like a random change - especially in a PR that claims to change a comment only
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2360633853)
I still think this should need a test as well, otherwise it just seems like a random change - especially in a PR that claims to change a comment only
🤔 janb84 reviewed a pull request: "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings"
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3241450515)
Concept ACK a9b211540b79832fb4e4f870d2f770cf03d35b11
This PR reorders the binary checks to after the installation step. And removes the code paths that are made superfluous by the reorg .
Open NIT/question on `libexec` inclusion.
Guix builds and the security tests runs now after install: ✅
<details>
This PR:
```shell
[101%] Built target test_bitcoin
-- Install configuration: "RelWithDebInfo"
-- Installing: /distsrc-base/distsrc-a9b211540b79-riscv64-linux-gnu/installed/bitcoin-a9
...
(https://github.com/bitcoin/bitcoin/pull/33247#pullrequestreview-3241450515)
Concept ACK a9b211540b79832fb4e4f870d2f770cf03d35b11
This PR reorders the binary checks to after the installation step. And removes the code paths that are made superfluous by the reorg .
Open NIT/question on `libexec` inclusion.
Guix builds and the security tests runs now after install: ✅
<details>
This PR:
```shell
[101%] Built target test_bitcoin
-- Install configuration: "RelWithDebInfo"
-- Installing: /distsrc-base/distsrc-a9b211540b79-riscv64-linux-gnu/installed/bitcoin-a9
...
💬 janb84 commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2360676127)
NIT: This only covers 1 of the 2 binary locations, nowadays we also have `libexec` should this not also be checked ?
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2360676127)
NIT: This only covers 1 of the 2 binary locations, nowadays we also have `libexec` should this not also be checked ?
🚀 achow101 merged a pull request: "coins: warn on oversized `-dbcache`"
(https://github.com/bitcoin/bitcoin/pull/33333)
(https://github.com/bitcoin/bitcoin/pull/33333)
💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2360677074)
```suggestion
tfm::format(std::cerr, "Fatal error: expected message not found in the debug log: '%s'\n", m_message);
```
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2360677074)
```suggestion
tfm::format(std::cerr, "Fatal error: expected message not found in the debug log: '%s'\n", m_message);
```
💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2360683563)
nit/unrelated: I'm surprised to see an `std::list` here, `std::vector` is likely faster even for random erases
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2360683563)
nit/unrelated: I'm surprised to see an `std::list` here, `std::vector` is likely faster even for random erases
👍 l0rinc approved a pull request: "test: don't throw from the destructor of DebugLogHelper"
(https://github.com/bitcoin/bitcoin/pull/33388#pullrequestreview-3241444220)
Core review ACK 2dcf0c69b8659886ecfc85b174e59cd7f210f5c0
I would also prefer adding quotes around the message but it's not a blocker.
(https://github.com/bitcoin/bitcoin/pull/33388#pullrequestreview-3241444220)
Core review ACK 2dcf0c69b8659886ecfc85b174e59cd7f210f5c0
I would also prefer adding quotes around the message but it's not a blocker.
💬 l0rinc commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2360670884)
That is my understanding as well, thanks
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2360670884)
That is my understanding as well, thanks
📝 john-moffett opened a pull request: "rpc: addpeeraddress: throw on invalid IP"
(https://github.com/bitcoin/bitcoin/pull/33430)
Right now we return an opaque `{"success" : false}` in `addpeeraddress` for an empty or invalid IP. This changes it to throw `RPC_CLIENT_INVALID_IP_OR_SUBNET` with the error message `Invalid IP address`. Tests updated to match.
(https://github.com/bitcoin/bitcoin/pull/33430)
Right now we return an opaque `{"success" : false}` in `addpeeraddress` for an empty or invalid IP. This changes it to throw `RPC_CLIENT_INVALID_IP_OR_SUBNET` with the error message `Invalid IP address`. Tests updated to match.
💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3309277841)
Looked into this @achow101 and you're right about the setup issues - they weren't legacy wallet specific but rather configuration problems.
I've addressed the three issues you identified:
1. Added self.uses_wallet = True to prevent -disablewallet
2. MiniWallet naturally avoids deprecated settxfee by using modern fee handling
3. MiniWallet sidesteps multiwallet complexity entirely
Regarding your concern about the --gen-test-data path being rarely executed: I've ensured the test runs wit
...
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3309277841)
Looked into this @achow101 and you're right about the setup issues - they weren't legacy wallet specific but rather configuration problems.
I've addressed the three issues you identified:
1. Added self.uses_wallet = True to prevent -disablewallet
2. MiniWallet naturally avoids deprecated settxfee by using modern fee handling
3. MiniWallet sidesteps multiwallet complexity entirely
Regarding your concern about the --gen-test-data path being rarely executed: I've ensured the test runs wit
...