💬 dergoegge commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1825460284)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1825460284)
Concept ACK
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1825462831)
> Our service rests on the first seen rule. Based on our experience a trx with confirmed inputs, reasonable fee, not seen conflicting trx for 5-7 seconds - is reliable. Also the trxs do not happen in a vacuum but rather in commercial exchanges with expected characteristics.
Again, you still have refused to answer my question: Have you actually tested full-RBF double spends against your service?
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1825462831)
> Our service rests on the first seen rule. Based on our experience a trx with confirmed inputs, reasonable fee, not seen conflicting trx for 5-7 seconds - is reliable. Also the trxs do not happen in a vacuum but rather in commercial exchanges with expected characteristics.
Again, you still have refused to answer my question: Have you actually tested full-RBF double spends against your service?
💬 hebasto commented on pull request "ci: Avoid toolset ambiguity that MSVC can't handle":
(https://github.com/bitcoin/bitcoin/pull/28905#issuecomment-1825472700)
> Do we know why this broke again? I guess this is something we'll just have to maintain forever?
I've done a bit more research.
In GHA Windows images, it is an accepted practice to have different versions of MSVC toolsets being installed side-by-side.
For example, the current image 20231115 has the following toolsets installed:
- 14.29.16.11
- 14.37.17.7
- 14.38.17.8
The [`ilammy/msvc-dev-cmd`](https://github.com/ilammy/msvc-dev-cmd) action chooses the latest compiler toolset ver
...
(https://github.com/bitcoin/bitcoin/pull/28905#issuecomment-1825472700)
> Do we know why this broke again? I guess this is something we'll just have to maintain forever?
I've done a bit more research.
In GHA Windows images, it is an accepted practice to have different versions of MSVC toolsets being installed side-by-side.
For example, the current image 20231115 has the following toolsets installed:
- 14.29.16.11
- 14.37.17.7
- 14.38.17.8
The [`ilammy/msvc-dev-cmd`](https://github.com/ilammy/msvc-dev-cmd) action chooses the latest compiler toolset ver
...
💬 theStack commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1404207499)
Good point indeed. To execute the steps build/encrypt/send-message atomically, I think introducing a new lock and acquiring it in the `send_message` method should do the trick? I.e.
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index a70682a768..59707471a6 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -164,6 +164,7 @@ class P2PConnection(asyncio.Protocol):
# The underlying tra
...
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1404207499)
Good point indeed. To execute the steps build/encrypt/send-message atomically, I think introducing a new lock and acquiring it in the `send_message` method should do the trick? I.e.
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index a70682a768..59707471a6 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -164,6 +164,7 @@ class P2PConnection(asyncio.Protocol):
# The underlying tra
...
💬 stratospher commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1404234919)
+1, good catch! when i tried this, just running `self.test_resource_exhaustion()` in `p2p_invalid messages.py` took too much time.
1. in v1, without locks - 0m22.265s
2. in v2, without locks - won't work
3. in v1, with locks - 0m22.101s
4. in v2, with locks - 8m45.916s
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1404234919)
+1, good catch! when i tried this, just running `self.test_resource_exhaustion()` in `p2p_invalid messages.py` took too much time.
1. in v1, without locks - 0m22.265s
2. in v2, without locks - won't work
3. in v1, with locks - 0m22.101s
4. in v2, with locks - 8m45.916s
💬 maflcko commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1404246109)
`*std::get_if` may be UB? In any case, it would be good to submit fuzz inputs that cover everything here.
(https://github.com/bitcoin/bitcoin/pull/28578#discussion_r1404246109)
`*std::get_if` may be UB? In any case, it would be good to submit fuzz inputs that cover everything here.
💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1825561375)
Rebased and addressed comments
- https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155488768: @martinus I took parts from your suggestion and added you as a co-author for the first commit. Thanks for the macro-magic. There's now only the `TRACEPOINT()` macro that works for tracepoints with 0 to 12 args.
- https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1140555021: I clarified unit tests and added a new test for a manual `TRACEPOINT_ACTIVE()` macro.
- https://github.com/bi
...
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-1825561375)
Rebased and addressed comments
- https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1155488768: @martinus I took parts from your suggestion and added you as a co-author for the first commit. Thanks for the macro-magic. There's now only the `TRACEPOINT()` macro that works for tracepoints with 0 to 12 args.
- https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1140555021: I clarified unit tests and added a new test for a manual `TRACEPOINT_ACTIVE()` macro.
- https://github.com/bi
...
📝 maflcko opened a pull request: "fuzz: Faster wallet_notifications target"
(https://github.com/bitcoin/bitcoin/pull/28933)
Avoid read/write from storage to speed the target up.
(https://github.com/bitcoin/bitcoin/pull/28933)
Avoid read/write from storage to speed the target up.
👍 willcl-ark approved a pull request: "Fee Estimator updates from Validation Interface/CScheduler thread"
(https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1746716540)
ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2
Glad to see the fee estimator being decoupled in this way and keen to see more development of additional estimators in the future e.g. as per #27995
I gave the code a review and it seems well thought-out to me at this stage; left a few nits which can be addressed in the case that you re-touch things.
(https://github.com/bitcoin/bitcoin/pull/28368#pullrequestreview-1746716540)
ACK 91504cbe0de2b74ef1aa2709761aaf0597ec66a2
Glad to see the fee estimator being decoupled in this way and keen to see more development of additional estimators in the future e.g. as per #27995
I gave the code a review and it seems well thought-out to me at this stage; left a few nits which can be addressed in the case that you re-touch things.
💬 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)