💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2176619287)
Made the following changes:
```diff
diff --git a/src/random.cpp b/src/random.cpp
index 71a2fcc781..39a1e3eab9 100644
--- a/src/random.cpp
+++ b/src/random.cpp
@@ -763,5 +763,12 @@ void RandomInit()
double MakeExponentiallyDistributed(uint64_t uniform) noexcept
{
- return -std::log1p(uniform * -5.42101086242752217e-20 /* -1/2^64 */);
+ // To convert uniform into an exponentially-distributed double, we use two steps:
+ // - Convert uniform into a uniformly-distributed do
...
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2176619287)
Made the following changes:
```diff
diff --git a/src/random.cpp b/src/random.cpp
index 71a2fcc781..39a1e3eab9 100644
--- a/src/random.cpp
+++ b/src/random.cpp
@@ -763,5 +763,12 @@ void RandomInit()
double MakeExponentiallyDistributed(uint64_t uniform) noexcept
{
- return -std::log1p(uniform * -5.42101086242752217e-20 /* -1/2^64 */);
+ // To convert uniform into an exponentially-distributed double, we use two steps:
+ // - Convert uniform into a uniformly-distributed do
...
📝 am-sq opened a pull request: "doc: loadwallet loads from relative walletdir"
(https://github.com/bitcoin/bitcoin/pull/30302)
## Why this change?
https://github.com/bitcoin/bitcoin/issues/30269 describes a need for documentation improvement with the `loadwallet` RPC. Namely, [some users have found](https://bitcoin.stackexchange.com/questions/123331/how-do-you-load-a-regtest-wallet) the usage description confusing when it comes to loading wallets that are not in the normal case of being in the default wallet directory.
The default wallet directory, depending on the machine OS, has the base directory defined here:
...
(https://github.com/bitcoin/bitcoin/pull/30302)
## Why this change?
https://github.com/bitcoin/bitcoin/issues/30269 describes a need for documentation improvement with the `loadwallet` RPC. Namely, [some users have found](https://bitcoin.stackexchange.com/questions/123331/how-do-you-load-a-regtest-wallet) the usage description confusing when it comes to loading wallets that are not in the normal case of being in the default wallet directory.
The default wallet directory, depending on the machine OS, has the base directory defined here:
...
💬 maflcko commented on pull request "doc: loadwallet loads from relative walletdir":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1644847206)
This is wrong. Absolute paths are allowed.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1644847206)
This is wrong. Absolute paths are allowed.
💬 Sjors commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2176675056)
TOTAL: 176948713 coins written to ../utxo-snapshots/utxo-840000.sqlite
Not sure if it's supposed to be deterministic, but here you go:
```
99450bac6e081e03751c064034b3d10e17b463ca56dfc35797fc6b5fe0de8ac4 ../utxo-snapshots/utxo-840000.sqlite
```
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-2176675056)
TOTAL: 176948713 coins written to ../utxo-snapshots/utxo-840000.sqlite
Not sure if it's supposed to be deterministic, but here you go:
```
99450bac6e081e03751c064034b3d10e17b463ca56dfc35797fc6b5fe0de8ac4 ../utxo-snapshots/utxo-840000.sqlite
```
💬 am-sq commented on pull request "doc: loadwallet loads from relative walletdir":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1644867028)
Thanks for the review. Is this description better?
```
You may pass in the absolute path of your wallet directory or .dat file. Otherwise, the interpreted path will be relative to the default wallet directory.
```
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1644867028)
Thanks for the review. Is this description better?
```
You may pass in the absolute path of your wallet directory or .dat file. Otherwise, the interpreted path will be relative to the default wallet directory.
```
💬 maflcko commented on pull request "doc: loadwallet loads from relative walletdir":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1644869283)
Do sqlite wallets allow `.dat` files to be passed, or is this only for BDB legacy wallets?
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1644869283)
Do sqlite wallets allow `.dat` files to be passed, or is this only for BDB legacy wallets?
💬 am-sq commented on pull request "doc: loadwallet loads from relative walletdir":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1644870338)
I can also add examples that load wallet using an absolute path.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1644870338)
I can also add examples that load wallet using an absolute path.
💬 mzumsande commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1644874521)
Removing the possibility of 0 garbage in `EncryptedP2PState` just to make this test nicer is a bit unfortunate in my opinion: If the node had a bug dealing with zero-length garbage, the test framework wouldn't be able to catch that anymore. Can't we change it locally just for the test cases who need it?
(https://github.com/bitcoin/bitcoin/pull/29431#discussion_r1644874521)
Removing the possibility of 0 garbage in `EncryptedP2PState` just to make this test nicer is a bit unfortunate in my opinion: If the node had a bug dealing with zero-length garbage, the test framework wouldn't be able to catch that anymore. Can't we change it locally just for the test cases who need it?
📝 theuni converted_to_draft a pull request: "depends: bump miniupnpc to 2.2.8"
(https://github.com/bitcoin/bitcoin/pull/30301)
Drops two of our patches that have been merged upstream and adjusts the other to deal with recent changes.
Follow-up from #30283. I can't vouch for the upstream changes here.
(https://github.com/bitcoin/bitcoin/pull/30301)
Drops two of our patches that have been merged upstream and adjusts the other to deal with recent changes.
Follow-up from #30283. I can't vouch for the upstream changes here.
💬 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?