💬 willcl-ark commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1403539773)
nit if you retouch bfcd401368fc0dc43827a8969a37b7e038d5ca79:
```suggestion
* Provides the block that was disconnected.
```
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1403539773)
nit if you retouch bfcd401368fc0dc43827a8969a37b7e038d5ca79:
```suggestion
* Provides the block that was disconnected.
```
💬 willcl-ark commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404174636)
micro nit, if you end up touching 714523918ba2b853fc69bee6b04a33ba0c828bf5 again:
```suggestion
// Drop transactions we were still watching, record fee estimations and unregister
```
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404174636)
micro nit, if you end up touching 714523918ba2b853fc69bee6b04a33ba0c828bf5 again:
```suggestion
// Drop transactions we were still watching, record fee estimations and unregister
```
💬 willcl-ark commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404159173)
In dff5ad3b9944cbb56126ba37a8da180d1327ba39
Worth adding a doxygen comment here? Even something simple like:
```cpp
/**
* Holds information about new transactions added to the mempool.
* In addition to TransactionInfo includes limit bypass, package, chain and parent info.
*/
```
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404159173)
In dff5ad3b9944cbb56126ba37a8da180d1327ba39
Worth adding a doxygen comment here? Even something simple like:
```cpp
/**
* Holds information about new transactions added to the mempool.
* In addition to TransactionInfo includes limit bypass, package, chain and parent info.
*/
```
💬 willcl-ark commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404305799)
In 714523918ba2b853fc69bee6b04a33ba0c828bf5
Would we want to use `fuzzed_data_provider.ConsumeBool()` for `m_submitted_in_package`?
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404305799)
In 714523918ba2b853fc69bee6b04a33ba0c828bf5
Would we want to use `fuzzed_data_provider.ConsumeBool()` for `m_submitted_in_package`?
💬 willcl-ark commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404169179)
In dff5ad3b9944cbb56126ba37a8da180d1327ba39
You've called this `m_from_disconnected_block`, but described it as whether the tx was added without enforcing mempool fee limits. This seems incorrect or confusing.
`SubmitPackage()` is calling this using `args.m_bypass_limits` so i think the comment is correct, and the variable should be renamed?
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1404169179)
In dff5ad3b9944cbb56126ba37a8da180d1327ba39
You've called this `m_from_disconnected_block`, but described it as whether the tx was added without enforcing mempool fee limits. This seems incorrect or confusing.
`SubmitPackage()` is calling this using `args.m_bypass_limits` so i think the comment is correct, and the variable should be renamed?
👋 brunoerg's pull request is ready for review: "contrib: add test for bucketing with asmap"
(https://github.com/bitcoin/bitcoin/pull/28869)
(https://github.com/bitcoin/bitcoin/pull/28869)
💬 brunoerg commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1825657203)
@fjahr, nice idea. Force-pushed adding it.
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1825657203)
@fjahr, nice idea. Force-pushed adding it.
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1404355662)
`const std::array a{x, y, z};` - at compile time the compiler knows the size of the array. `a.size()` can be used to define the size of other arrays, e.g. `std::array<int, a.size()> anoher;`. This is what matters. Whether the contents of the array is a compile time constant (the pointers), I am not sure.
(https://github.com/bitcoin/bitcoin/pull/28926#discussion_r1404355662)
`const std::array a{x, y, z};` - at compile time the compiler knows the size of the array. `a.size()` can be used to define the size of other arrays, e.g. `std::array<int, a.size()> anoher;`. This is what matters. Whether the contents of the array is a compile time constant (the pointers), I am not sure.
🚀 fanquake merged a pull request: "ci: remove `python3-setuptools` from macOS build deps"
(https://github.com/bitcoin/bitcoin/pull/28932)
(https://github.com/bitcoin/bitcoin/pull/28932)
💬 vasild commented on pull request "rpc: add 'getnetmsgstats' RPC":
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1825696764)
> It might be better to add a `totals` field to all groups, in every dimension. In that case I would also have the default `getnetmsgstats` return something a bit more interesting, e.g. the equivalent of `getnetmsgstats '["message_type"]`.
That would be interesting. However it would further complicate this PR. The grouping/aggregating part I think is already a bit convoluted. Maybe it would make sense to drop it and print the full output without any grouping in this PR and do the grouping in
...
(https://github.com/bitcoin/bitcoin/pull/28926#issuecomment-1825696764)
> It might be better to add a `totals` field to all groups, in every dimension. In that case I would also have the default `getnetmsgstats` return something a bit more interesting, e.g. the equivalent of `getnetmsgstats '["message_type"]`.
That would be interesting. However it would further complicate this PR. The grouping/aggregating part I think is already a bit convoluted. Maybe it would make sense to drop it and print the full output without any grouping in this PR and do the grouping in
...
👍 dergoegge approved a pull request: "fuzz: Faster wallet_notifications target"
(https://github.com/bitcoin/bitcoin/pull/28933#pullrequestreview-1747977684)
Code review ACK fa4fc99b6e2a13736521e6bca1f626ba1d2f59e3
With the mocked database this shouldn't be going to disk at all now, right?
(https://github.com/bitcoin/bitcoin/pull/28933#pullrequestreview-1747977684)
Code review ACK fa4fc99b6e2a13736521e6bca1f626ba1d2f59e3
With the mocked database this shouldn't be going to disk at all now, right?
💬 maflcko commented on pull request "fuzz: Faster wallet_notifications target":
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825707017)
> With the mocked database this shouldn't be going to disk at all now, right?
Yeah, I guess so. (A datadir is still created, though)
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825707017)
> With the mocked database this shouldn't be going to disk at all now, right?
Yeah, I guess so. (A datadir is still created, though)
💬 maflcko commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1825713535)
See https://github.com/bitcoin/bitcoin/pull/28933
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1825713535)
See https://github.com/bitcoin/bitcoin/pull/28933
💬 dergoegge commented on pull request "fuzz: Faster wallet_notifications target":
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825717030)
> A datadir is still created, though
Is that necessary or just a side effect from using `TestingSetup`?
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825717030)
> A datadir is still created, though
Is that necessary or just a side effect from using `TestingSetup`?
💬 brunoerg commented on pull request "fuzz: Faster wallet_notifications target":
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825721587)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825721587)
Concept ACK
💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1404392341)
@stratospher: Seems like the long run-time in case 4. is primarily caused by the slow ChaCha20 implementation in Python. On my machine, encrypting 4MB of data (as done in `test_resource_exhaustion`, but repeated 80 times) takes already more than half a minute:
```
$ ipython
Python 3.10.13 (main, Oct 5 2023, 16:21:31) [Clang 13.0.0 ]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.13.2 -- An enhanced Interactive Python. Type '?' for help.
In [1]: from test.functi
...
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1404392341)
@stratospher: Seems like the long run-time in case 4. is primarily caused by the slow ChaCha20 implementation in Python. On my machine, encrypting 4MB of data (as done in `test_resource_exhaustion`, but repeated 80 times) takes already more than half a minute:
```
$ ipython
Python 3.10.13 (main, Oct 5 2023, 16:21:31) [Clang 13.0.0 ]
Type 'copyright', 'credits' or 'license' for more information
IPython 8.13.2 -- An enhanced Interactive Python. Type '?' for help.
In [1]: from test.functi
...
💬 maflcko commented on pull request "fuzz: Faster wallet_notifications target":
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825740251)
> Is that necessary or just a side effect from using `TestingSetup`?
Both. For one, `validation` can not exist without datadir. Also, it is created in `TestingSetup`. But it shouldn't cause any issues, because it will be empty.
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825740251)
> Is that necessary or just a side effect from using `TestingSetup`?
Both. For one, `validation` can not exist without datadir. Also, it is created in `TestingSetup`. But it shouldn't cause any issues, because it will be empty.
💬 dergoegge commented on pull request "fuzz: Faster wallet_notifications target":
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825742736)
> But it shouldn't cause any issues, because it will be empty.
Empty dirs aren't of size zero and #28811 would still apply (given more timeouts/crashes).
It's a bit of a shame if it's always empty to still create it, but not an issue for this PR in any case.
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825742736)
> But it shouldn't cause any issues, because it will be empty.
Empty dirs aren't of size zero and #28811 would still apply (given more timeouts/crashes).
It's a bit of a shame if it's always empty to still create it, but not an issue for this PR in any case.
💬 furszy commented on issue "wallet RPC to double-check the calculated balance":
(https://github.com/bitcoin/bitcoin/issues/28898#issuecomment-1825745912)
> the rescan won't find the txs if the birthdate is wrong
The birth time check resides on the 'block connected' event handler. It does not affect the `rescanblockchain` functionality. Users can utilize it to identify missing transactions.
> see also #28897
Fixed it few days ago, see #28920.
> Moreover, on hardware without parallel cores, building the block filter index is not going to be sped up
That's true, but it's not a reason not to do it. The entire node is multi-threaded, an
...
(https://github.com/bitcoin/bitcoin/issues/28898#issuecomment-1825745912)
> the rescan won't find the txs if the birthdate is wrong
The birth time check resides on the 'block connected' event handler. It does not affect the `rescanblockchain` functionality. Users can utilize it to identify missing transactions.
> see also #28897
Fixed it few days ago, see #28920.
> Moreover, on hardware without parallel cores, building the block filter index is not going to be sped up
That's true, but it's not a reason not to do it. The entire node is multi-threaded, an
...
💬 maflcko commented on pull request "fuzz: Faster wallet_notifications target":
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825765383)
Are there any timeouts left at this point? If yes, it may be good to report them.
(https://github.com/bitcoin/bitcoin/pull/28933#issuecomment-1825765383)
Are there any timeouts left at this point? If yes, it may be good to report them.
💬 maflcko commented on issue "wallet RPC to double-check the calculated balance":
(https://github.com/bitcoin/bitcoin/issues/28898#issuecomment-1825767907)
> The birth time check resides on the 'block connected' event handler. It does not affect the `rescanblockchain` functionality. Users can utilize it to identify missing transactions.
Ah, in that case, a warning can be printed to explain that `rescanblockchain` can be used to recover the transaction history (and balance).
(https://github.com/bitcoin/bitcoin/issues/28898#issuecomment-1825767907)
> The birth time check resides on the 'block connected' event handler. It does not affect the `rescanblockchain` functionality. Users can utilize it to identify missing transactions.
Ah, in that case, a warning can be printed to explain that `rescanblockchain` can be used to recover the transaction history (and balance).