💬 pinheadmz commented on pull request "blockstorage: add an assert to avoid running oom with `-fastprune`":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1123567195)
This cleanup is already addressed in #27039 ;-)
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1123567195)
This cleanup is already addressed in #27039 ;-)
👍 ryanofsky approved a pull request: "refactor: RPC: pass named argument value as string_view"
(https://github.com/bitcoin/bitcoin/pull/26612)
Code review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c
I think the followup ideas in the PR description sound very good too
(https://github.com/bitcoin/bitcoin/pull/26612)
Code review ACK 545ff924ab6303ffabd91fdfc4f0a4962daf133c
I think the followup ideas in the PR description sound very good too
💬 brunoerg commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1123587538)
I prefer not having `=manual`, it would add more stuff to the code and I believe it might complicate for the user, it's the default behavior, not sure why someone would want to specify it.
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1123587538)
I prefer not having `=manual`, it would add more stuff to the code and I believe it might complicate for the user, it's the default behavior, not sure why someone would want to specify it.
💬 brunoerg commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1123591071)
What would be the difference between `resolvedAddress` and `m_node_address`?
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1123591071)
What would be the difference between `resolvedAddress` and `m_node_address`?
💬 brunoerg commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1123596809)
nit: you could create `manual_name` and `manual_block_relay_name` after checking the connection type with "remove":
```diff
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 6155af323..9d87d89e3 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -304,11 +304,14 @@ static RPCHelpMan addnode()
throw std::runtime_error(
self.ToString());
}
- const std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};
- const std::string manual_blo
...
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1123596809)
nit: you could create `manual_name` and `manual_block_relay_name` after checking the connection type with "remove":
```diff
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 6155af323..9d87d89e3 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -304,11 +304,14 @@ static RPCHelpMan addnode()
throw std::runtime_error(
self.ToString());
}
- const std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};
- const std::string manual_blo
...
💬 jamesob commented on pull request "assumeutxo: background validation completion":
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1452426488)
> In the pull description, could drop or update "Since it's dependent on the commits in #25667, I'm opening this as a draft."
Done, thanks.
(https://github.com/bitcoin/bitcoin/pull/25740#issuecomment-1452426488)
> In the pull description, could drop or update "Since it's dependent on the commits in #25667, I'm opening this as a draft."
Done, thanks.
💬 ishaanam commented on pull request "wallet: ensure the wallet is unlocked when needed for rescanning":
(https://github.com/bitcoin/bitcoin/pull/26347#discussion_r1123601921)
> Maybe the timeout can be increased to 999999?
Yes, the timeout before `sethdseed` can be increased to 999999, followed by running `walletlock`. I can open a follow-up PR for this.
> Also I wonder why you picked a timeout of 3 below, or why walletpassphrase needs to be called at all twice in a row?
3 was used because it needed to be a number that was large enough so that the scan would begin, but small enough so that the timeout would occur while the rescan is in progress. However, this
...
(https://github.com/bitcoin/bitcoin/pull/26347#discussion_r1123601921)
> Maybe the timeout can be increased to 999999?
Yes, the timeout before `sethdseed` can be increased to 999999, followed by running `walletlock`. I can open a follow-up PR for this.
> Also I wonder why you picked a timeout of 3 below, or why walletpassphrase needs to be called at all twice in a row?
3 was used because it needed to be a number that was large enough so that the scan would begin, but small enough so that the timeout would occur while the rescan is in progress. However, this
...
✅ fanquake closed an issue: "Coin Controll for Unconfirmed Outputs"
(https://github.com/bitcoin/bitcoin/issues/27190)
(https://github.com/bitcoin/bitcoin/issues/27190)
💬 brunoerg commented on pull request "p2p, rpc: Manual block-relay-only connections with addnode":
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1123652674)
perhaps there is a way to simplify it:
```diff
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 6155af323..b9e082a63 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -304,22 +304,19 @@ static RPCHelpMan addnode()
throw std::runtime_error(
self.ToString());
}
- const std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};
- const std::string manual_block_relay_name{ConnectionTypeAsString(ConnectionType::MANUAL_BLOCK_RELAY)};
+
...
(https://github.com/bitcoin/bitcoin/pull/24170#discussion_r1123652674)
perhaps there is a way to simplify it:
```diff
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 6155af323..b9e082a63 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -304,22 +304,19 @@ static RPCHelpMan addnode()
throw std::runtime_error(
self.ToString());
}
- const std::string manual_name{ConnectionTypeAsString(ConnectionType::MANUAL)};
- const std::string manual_block_relay_name{ConnectionTypeAsString(ConnectionType::MANUAL_BLOCK_RELAY)};
+
...
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123661658)
yeah could do that, but we wouldn't win much really. The commit that touches
this method signature is the biggest one of the PR, so we would add more diff
for no real gain.
Better to leave it for the next PR on the line. I have few that will come right after
this one anyway.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123661658)
yeah could do that, but we wouldn't win much really. The commit that touches
this method signature is the biggest one of the PR, so we would add more diff
for no real gain.
Better to leave it for the next PR on the line. I have few that will come right after
this one anyway.
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123685274)
Ok understood it, the `mixed` term is also used for the `allow_mixed_output_types` flag. Which is not from this PR.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123685274)
Ok understood it, the `mixed` term is also used for the `allow_mixed_output_types` flag. Which is not from this PR.
📝 theuni opened a pull request: "util: add missing include and fix function signature"
(https://github.com/bitcoin/bitcoin/pull/27192)
ping @hebasto
Discovered while testing pre-compiled header support with CMake: https://github.com/theuni/bitcoin/commits/cmake-pch-poc. Compilation of that branch fails without this fix and succeeds with it.
Similar to the fix in #27144.
The problem of having a default argument in the definition was masked by the missing include. Using PCH forces that include, so we end up with the compiler error we should've been getting all along.
(https://github.com/bitcoin/bitcoin/pull/27192)
ping @hebasto
Discovered while testing pre-compiled header support with CMake: https://github.com/theuni/bitcoin/commits/cmake-pch-poc. Compilation of that branch fails without this fix and succeeds with it.
Similar to the fix in #27144.
The problem of having a default argument in the definition was masked by the missing include. Using PCH forces that include, so we end up with the compiler error we should've been getting all along.
🚀 fanquake merged a pull request: "doc: Update Transifex links and slug format in Release Process"
(https://github.com/bitcoin/bitcoin/pull/27183)
(https://github.com/bitcoin/bitcoin/pull/27183)
💬 sipa commented on issue "Use of a wallet shouldn't be blocked in prune mode ("wallet loading failed... beyond pruned data")":
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1452563398)
@willcl-ark That still won't work when the pruning point is later than the wallet birth date.
(https://github.com/bitcoin/bitcoin/issues/27188#issuecomment-1452563398)
@willcl-ark That still won't work when the pruning point is later than the wallet birth date.
👍 ryanofsky approved a pull request: "refactor: Use move semantics instead of custom swap functions"
(https://github.com/bitcoin/bitcoin/pull/26749)
Conditional code review ACK 142718890e718397e0026c315c8940102b9657ce, assuming this shouldn't use emplace_back https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123317784 or avoid temporary vector allocations https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123429117.
I mostly reviewed the overall PR diff and didn't review individual commits in detail since it was easier to think about class definitions with only move operations or only swap operations, not combinations of
...
(https://github.com/bitcoin/bitcoin/pull/26749)
Conditional code review ACK 142718890e718397e0026c315c8940102b9657ce, assuming this shouldn't use emplace_back https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123317784 or avoid temporary vector allocations https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1123429117.
I mostly reviewed the overall PR diff and didn't review individual commits in detail since it was easier to think about class definitions with only move operations or only swap operations, not combinations of
...
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1452678639)
Thanks @S3RK for the review.
Updated per feedback ([small diff](https://github.com/bitcoin/bitcoin/compare/0bb90cabc8e2c6bbbc3c52ec1170e725435b625d..ae0dd2becd4fa01666793c3f48e8725d5e585c30)), tackled the following points that made sense for me to include here:
* Renamed `OutputGroups` to `OutputGroupTypeMap` to reflect what the struct really does and differentiate it from the inner struct.
* Corrected the `OutputGroups`, now `OutputGroupTypeMap`, members comments.
* And tackled @Xekyo’s
...
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1452678639)
Thanks @S3RK for the review.
Updated per feedback ([small diff](https://github.com/bitcoin/bitcoin/compare/0bb90cabc8e2c6bbbc3c52ec1170e725435b625d..ae0dd2becd4fa01666793c3f48e8725d5e585c30)), tackled the following points that made sense for me to include here:
* Renamed `OutputGroups` to `OutputGroupTypeMap` to reflect what the struct really does and differentiate it from the inner struct.
* Corrected the `OutputGroups`, now `OutputGroupTypeMap`, members comments.
* And tackled @Xekyo’s
...
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123825816)
Ended up renaming `OutputGroups` to `OutputGroupTypeMap` to reflect what the struct really does and differentiate it from the inner struct.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123825816)
Ended up renaming `OutputGroups` to `OutputGroupTypeMap` to reflect what the struct really does and differentiate it from the inner struct.
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123826202)
Fixed the "filter" comment mention.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1123826202)
Fixed the "filter" comment mention.
👍 furszy approved a pull request: "wallet: Migrate wallets that are not in a wallet dir"
(https://github.com/bitcoin/bitcoin/pull/26740)
rebase ACK 52634dba
(https://github.com/bitcoin/bitcoin/pull/26740)
rebase ACK 52634dba
💬 mzumsande commented on pull request "blockstorage: add an assert to avoid running oom with `-fastprune`":
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1124047278)
Good, should make rebasing easy! Here, it's not a cleanup but needed to use `max_blockfile_size` in the added assert.
(https://github.com/bitcoin/bitcoin/pull/27191#discussion_r1124047278)
Good, should make rebasing easy! Here, it's not a cleanup but needed to use `max_blockfile_size` in the added assert.