💬 theuni commented on pull request "depends: bump miniupnpc to 2.2.8":
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2176713996)
Marked as draft #30283 is merged.
(https://github.com/bitcoin/bitcoin/pull/30301#issuecomment-2176713996)
Marked as draft #30283 is merged.
💬 mzumsande commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1644893828)
Looks good (except for the compiler issue with `Join()`) - I think it would be helpful to not only log the reason, but also return them with the RPC - for that, `ActivateSnapshot()` would need to return it, possibly as `util::Result` as suggested by ryanofsky, but that could be a follow-up.
Though for now, it might be helpful to expand the error ("Unable to load UTXO snapshot") to point the user to the debug log for the reason.
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1644893828)
Looks good (except for the compiler issue with `Join()`) - I think it would be helpful to not only log the reason, but also return them with the RPC - for that, `ActivateSnapshot()` would need to return it, possibly as `util::Result` as suggested by ryanofsky, but that could be a follow-up.
Though for now, it might be helpful to expand the error ("Unable to load UTXO snapshot") to point the user to the debug log for the reason.
🤔 sipa reviewed a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2126279596)
Just a quick look so far.
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2126279596)
Just a quick look so far.
💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644928807)
In commit "net: Add PCP and NATPMP implementation":
Nit: `check_packet({response, recvsz})` should work too (if no type is listed, the corresponding constructor of the declared type is used).
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644928807)
In commit "net: Add PCP and NATPMP implementation":
Nit: `check_packet({response, recvsz})` should work too (if no type is listed, the corresponding constructor of the declared type is used).
💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644926540)
In commit "net: Add PCP and NATPMP implementation"
Put a `namespace {` ... `} // namespace` around all of the functions/definitions/constants that are not in the .h file, and drop the `static` (just a nit, but anonymous namespaces are considered more modern).
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644926540)
In commit "net: Add PCP and NATPMP implementation"
Put a `namespace {` ... `} // namespace` around all of the functions/definitions/constants that are not in the .h file, and drop the `static` (just a nit, but anonymous namespaces are considered more modern).
💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644962602)
In commit "net: Add PCP and NATPMP implementation"
Nit, if you want somewhat more brevity:
```c++
using namespace std::chrono_literals;
std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &gateway, uint16_t port, uint32_t lifetime, int num_tries = 3, std::chrono::milliseconds timeout_per_try = 1s);
```
(and below)
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644962602)
In commit "net: Add PCP and NATPMP implementation"
Nit, if you want somewhat more brevity:
```c++
using namespace std::chrono_literals;
std::variant<MappingResult, MappingError> NATPMPRequestPortMap(const CNetAddr &gateway, uint16_t port, uint32_t lifetime, int num_tries = 3, std::chrono::milliseconds timeout_per_try = 1s);
```
(and below)
💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644941149)
In commit "net: Add PCP and NATPMP implementation"
Is this `internal` address expected to be different than the `gateway` variable that's passed in? It seems this `internal` address is only used to convert to a `CService` at return time from this function, so if it's equal to `gateway`, this seems like a very roundabout way of getting there.
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644941149)
In commit "net: Add PCP and NATPMP implementation"
Is this `internal` address expected to be different than the `gateway` variable that's passed in? It seems this `internal` address is only used to convert to a `CService` at return time from this function, so if it's equal to `gateway`, this seems like a very roundabout way of getting there.
💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644929901)
In commit "net: Add PCP and NATPMP implementation"
The `const` before `Span<const uint8_t>` has no effect (it's an argument that's passed by value). You can drop it while still keeping the one in the actual lambdas passed to this function (if you want to prevent the body of those lambdas from modifying the copy of the Span they receive).
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644929901)
In commit "net: Add PCP and NATPMP implementation"
The `const` before `Span<const uint8_t>` has no effect (it's an argument that's passed by value). You can drop it while still keeping the one in the actual lambdas passed to this function (if you want to prevent the body of those lambdas from modifying the copy of the Span they receive).
💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644965614)
In commit "net: Add PCP and NATPMP implementation"
Nit: space after type
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1644965614)
In commit "net: Add PCP and NATPMP implementation"
Nit: space after type
💬 MarcusMWilliams commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-2176895321)
Tire & Rim
On Tue, Jun 18, 2024, 3:07 PM DrahtBot ***@***.***> wrote:
> 🚧 At least one of the CI tasks failed. Make sure to run all tests
> locally, according to the
> documentation.
>
> Possibly this is due to a silent merge conflict (the changes in this pull
> request being
> incompatible with the current code in the target branch). If so, make sure
> to rebase on the latest
> commit of the target branch.
>
> Leave a comment here, if you need help tracking down a confusing fai
...
(https://github.com/bitcoin/bitcoin/pull/28676#issuecomment-2176895321)
Tire & Rim
On Tue, Jun 18, 2024, 3:07 PM DrahtBot ***@***.***> wrote:
> 🚧 At least one of the CI tasks failed. Make sure to run all tests
> locally, according to the
> documentation.
>
> Possibly this is due to a silent merge conflict (the changes in this pull
> request being
> incompatible with the current code in the target branch). If so, make sure
> to rebase on the latest
> commit of the target branch.
>
> Leave a comment here, if you need help tracking down a confusing fai
...
💬 ryanofsky commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1645010762)
In commit "refactor: PSBTError::MISSING_INPUTS" (b2b4b932572c5bb1ffa3fc4f34e17130348fbc24)
Does the name INPUTS_MISSING_OR_SPENT actually make sense to describe these two errors from FillPSBT? I'm not sure, but it seems these errors would only happen if the PSBT data was not internally consistent, so maybe it would be more appropriate to call this error something like PSBTError::INPUTS_INVALID?
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1645010762)
In commit "refactor: PSBTError::MISSING_INPUTS" (b2b4b932572c5bb1ffa3fc4f34e17130348fbc24)
Does the name INPUTS_MISSING_OR_SPENT actually make sense to describe these two errors from FillPSBT? I'm not sure, but it seems these errors would only happen if the PSBT data was not internally consistent, so maybe it would be more appropriate to call this error something like PSBTError::INPUTS_INVALID?
🤔 stickies-v reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2125109551)
Approach ACK 63923c8da686da42f522771f338ea8f2a4f4e568
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2125109551)
Approach ACK 63923c8da686da42f522771f338ea8f2a4f4e568
💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1644210320)
nit: declaring `requested_num_elems` as a `uint32_t` would seem more intuitive:
<details>
<summary>git diff on 8960bc986b</summary>
```diff
diff --git a/src/cuckoocache.h b/src/cuckoocache.h
index c9bd45befc..85556a4da5 100644
--- a/src/cuckoocache.h
+++ b/src/cuckoocache.h
@@ -364,10 +364,10 @@ public:
*/
std::pair<uint32_t, size_t> setup_bytes(size_t bytes)
{
- size_t requested_num_elems = bytes / sizeof(Element);
- if (std::numeric_limits<uint32_
...
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1644210320)
nit: declaring `requested_num_elems` as a `uint32_t` would seem more intuitive:
<details>
<summary>git diff on 8960bc986b</summary>
```diff
diff --git a/src/cuckoocache.h b/src/cuckoocache.h
index c9bd45befc..85556a4da5 100644
--- a/src/cuckoocache.h
+++ b/src/cuckoocache.h
@@ -364,10 +364,10 @@ public:
*/
std::pair<uint32_t, size_t> setup_bytes(size_t bytes)
{
- size_t requested_num_elems = bytes / sizeof(Element);
- if (std::numeric_limits<uint32_
...
💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1645009465)
If I understand correctly, `m_script_execution_cache_hasher` is a pre-initialized hasher that we want to copy every time we use it, and we definitely don't want to modify in-place. I think having it as a public struct member is not very robust to enforce that, perhaps better to use a setter here? E.g. something like:
<details>
<summary>git diff on 63923c8da6</summary>
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 39ab7b56ab..600c427c93 100644
--- a/src/validation.c
...
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1645009465)
If I understand correctly, `m_script_execution_cache_hasher` is a pre-initialized hasher that we want to copy every time we use it, and we definitely don't want to modify in-place. I think having it as a public struct member is not very robust to enforce that, perhaps better to use a setter here? E.g. something like:
<details>
<summary>git diff on 63923c8da6</summary>
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 39ab7b56ab..600c427c93 100644
--- a/src/validation.c
...
💬 stickies-v commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1644415772)
To minimize future footguns by accidentally passing a cache by value, perhaps best to disable the copy constructor and copy assignment operator? (+ same for `CSignatureCache` in 63923c8da686da42f522771f338ea8f2a4f4e568)
<details>
<summary>git diff on 1409fa4f41</summary>
```diff
diff --git a/src/validation.h b/src/validation.h
index 9f858bff1d..90f484bc9f 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -369,6 +369,10 @@ static_assert(std::is_nothrow_destructible_v<CScriptChec
...
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1644415772)
To minimize future footguns by accidentally passing a cache by value, perhaps best to disable the copy constructor and copy assignment operator? (+ same for `CSignatureCache` in 63923c8da686da42f522771f338ea8f2a4f4e568)
<details>
<summary>git diff on 1409fa4f41</summary>
```diff
diff --git a/src/validation.h b/src/validation.h
index 9f858bff1d..90f484bc9f 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -369,6 +369,10 @@ static_assert(std::is_nothrow_destructible_v<CScriptChec
...
💬 hebasto commented on pull request "[DO NOT MERGE] Erlay: bandwidth-efficient transaction relay protocol (Full implementation)":
(https://github.com/bitcoin/bitcoin/pull/30277#issuecomment-2176997916)
@sr-gi
Feel free to grab https://github.com/hebasto/bitcoin/commit/ffed2ab27465e4e3888b4139ca50db1226a66ec5 to fix the MSVC build.
(https://github.com/bitcoin/bitcoin/pull/30277#issuecomment-2176997916)
@sr-gi
Feel free to grab https://github.com/hebasto/bitcoin/commit/ffed2ab27465e4e3888b4139ca50db1226a66ec5 to fix the MSVC build.
💬 AngusP commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1645152984)
Addressed in 8df9d16
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1645152984)
Addressed in 8df9d16
💬 AngusP commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1645153389)
Addressed in 8df9d16
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1645153389)
Addressed in 8df9d16
🤔 furszy reviewed a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2126669884)
Just a small comment; you don't have to take it.
I believe this PR could get merged quite fast if you leave 89503710386f52d9162f67fcd707eceffa954faa for a follow-up and make `IsMine` compatible with `LegacyDataSPKM` in this PR.
I'm suggesting this because this PR starts testing the newly introduced BDB reader class and allows people to use it, which is great since we want it polished for the next release. The `IsMine` removal is also nice but I wouldn't miss a deadline for it.
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2126669884)
Just a small comment; you don't have to take it.
I believe this PR could get merged quite fast if you leave 89503710386f52d9162f67fcd707eceffa954faa for a follow-up and make `IsMine` compatible with `LegacyDataSPKM` in this PR.
I'm suggesting this because this PR starts testing the newly introduced BDB reader class and allows people to use it, which is great since we want it polished for the next release. The `IsMine` removal is also nice but I wouldn't miss a deadline for it.
💬 1440000bytes commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2177227865)
Recently 27.1 was released and nothing was include din DNS seed.. Sad and Cant do anything,
It is less likely they will secure domain in a domain takeover attack.
I do not trust them and adding one more including improves security.
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2177227865)
Recently 27.1 was released and nothing was include din DNS seed.. Sad and Cant do anything,
It is less likely they will secure domain in a domain takeover attack.
I do not trust them and adding one more including improves security.