💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268183969)
hm see https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1239761122 not sure what to do
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268183969)
hm see https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1239761122 not sure what to do
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268194341)
> I also ran a small unit test through perf where there were many writes and found that std::ftell dominated with std::fwrite close behind
For me fwrite dominated over ftell, but the most time was spent on XOR:
```
ROUTINE ====================== AutoFile::write in src/streams.cpp
49 100 Total samples (flat / cumulative)
. . 40: nSize -= nNow;
. . 41: }
. . 42: }
. . 43:
. . 44: void AutoFile::write(Spa
...
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268194341)
> I also ran a small unit test through perf where there were many writes and found that std::ftell dominated with std::fwrite close behind
For me fwrite dominated over ftell, but the most time was spent on XOR:
```
ROUTINE ====================== AutoFile::write in src/streams.cpp
49 100 Total samples (flat / cumulative)
. . 40: nSize -= nNow;
. . 41: }
. . 42: }
. . 43:
. . 44: void AutoFile::write(Spa
...
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268195960)
I am hoping to nuke `ftell` completely, but that would require removing the `Get` member function
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268195960)
I am hoping to nuke `ftell` completely, but that would require removing the `Get` member function
🤔 ismaelsadeeq reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1537066023)
Light code review ACK for commit 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6:
As I continue familiarizing myself with the code, my understanding is that this PR addresses follow-up nits up to a3b178f3c082453120e816945cea220bc7397ee5. Additionally, it introduces the use of `MiniMiner` to calculate the necessary fees required for fee bumping unconfirmed inputs to a target fee rate. The calculated fee bumps values are then added to the transaction's fee to account for the ancestor fee bump and reac
...
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1537066023)
Light code review ACK for commit 0f6c13665c4ab7d4928ff0fa63c4e755667f7fd6:
As I continue familiarizing myself with the code, my understanding is that this PR addresses follow-up nits up to a3b178f3c082453120e816945cea220bc7397ee5. Additionally, it introduces the use of `MiniMiner` to calculate the necessary fees required for fee bumping unconfirmed inputs to a target fee rate. The calculated fee bumps values are then added to the transaction's fee to account for the ancestor fee bump and reac
...
💬 ismaelsadeeq commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268063062)
We can use `CFeeRate(transaction_iter.value()->GetModFeesWithAncestors(), transaction_iter.value()->GetSizeWithAncestors())` to get the ancestor fee rate why are we calculating manually?
Like here ```tx6_anc_feerate = CFeeRate(high_fee + low_fee + med_fee, tx_vsizes[4] + tx_vsizes[5] + tx_vsizes[6]);``` and other places?
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268063062)
We can use `CFeeRate(transaction_iter.value()->GetModFeesWithAncestors(), transaction_iter.value()->GetSizeWithAncestors())` to get the ancestor fee rate why are we calculating manually?
Like here ```tx6_anc_feerate = CFeeRate(high_fee + low_fee + med_fee, tx_vsizes[4] + tx_vsizes[5] + tx_vsizes[6]);``` and other places?
💬 ismaelsadeeq commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268057988)
In 0c4ff4921087baf4ee00995be910241c3595cbe9
> This commit matches the
names to the transactions’ vector indices for better readability
This only applies to `miniminer_overlap`
`miniminer_1p1c` test case transactions name still starts at 1.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268057988)
In 0c4ff4921087baf4ee00995be910241c3595cbe9
> This commit matches the
names to the transactions’ vector indices for better readability
This only applies to `miniminer_overlap`
`miniminer_1p1c` test case transactions name still starts at 1.
💬 ismaelsadeeq commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268098822)
We can have something like
```python
def assert_spends_only_parents(self, tx, parent_txids):
number_inputs = len(tx["decoded"]["vin"])
assert_equal(number_inputs, len(parent_txids))
for i in range(number_inputs):
txid_of_input = tx["decoded"]["vin"][i]["txid"]
assert txid_of_input in parent_txids
```
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268098822)
We can have something like
```python
def assert_spends_only_parents(self, tx, parent_txids):
number_inputs = len(tx["decoded"]["vin"])
assert_equal(number_inputs, len(parent_txids))
for i in range(number_inputs):
txid_of_input = tx["decoded"]["vin"][i]["txid"]
assert txid_of_input in parent_txids
```
💬 ismaelsadeeq commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268094484)
`fee` is an optional variable, is it safe to just increment it, as it may be `nullptr`
```
if (num) {
*num += bump_fee;
}
```
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268094484)
`fee` is an optional variable, is it safe to just increment it, as it may be `nullptr`
```
if (num) {
*num += bump_fee;
}
```
💬 ismaelsadeeq commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268085294)
nit: feel free to ignore
slightly more code and less compact, but I think it is better to have a separate function for decrement and increment, its clear and more explicit and you don't have to negate.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1268085294)
nit: feel free to ignore
slightly more code and less compact, but I think it is better to have a separate function for decrement and increment, its clear and more explicit and you don't have to negate.
💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268219236)
Done
(https://github.com/bitcoin/bitcoin/pull/27499#discussion_r1268219236)
Done
💬 vasild commented on pull request "sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es":
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-1642275801)
`ebfe47e594...38c0d628e0`: rebase due to conflicts
@hebasto, if the purpose of the low level code is to make the higher level code more robust and less bug-prone, then I think `Synced` achieves this. The pattern:
```cpp
Mutex m;
Type my_variable GUARDED_BY(m);
```
is very common. The `Synced` structure encapsulates both, enforces the idiom and makes it impossible to "to abuse the mutex and start using it to protect some more, possibly unrelated stuff".
It also helps remove `Global
...
(https://github.com/bitcoin/bitcoin/pull/25390#issuecomment-1642275801)
`ebfe47e594...38c0d628e0`: rebase due to conflicts
@hebasto, if the purpose of the low level code is to make the higher level code more robust and less bug-prone, then I think `Synced` achieves this. The pattern:
```cpp
Mutex m;
Type my_variable GUARDED_BY(m);
```
is very common. The `Synced` structure encapsulates both, enforces the idiom and makes it impossible to "to abuse the mutex and start using it to protect some more, possibly unrelated stuff".
It also helps remove `Global
...
💬 furszy commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268231932)
Need to scope the `ASSERT_DEBUG_LOG` macro.
It expands to a `DebugLogHelper`, which has the check in the destructor. Also, this will swallow other logging errors as well if it is not scoped.
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268231932)
Need to scope the `ASSERT_DEBUG_LOG` macro.
It expands to a `DebugLogHelper`, which has the check in the destructor. Also, this will swallow other logging errors as well if it is not scoped.
💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268239265)
I think it must have been a fluke because when I synced up to a little past block ~200K, `fwrite` was in the lead and `ftell` and xor were about tied at ~3% each of the total time spent. I wrote a test here https://github.com/Crypt-iQ/bitcoin/commit/bf924d6d2d4020e799796c0751ac64c892ae8e6b that requires the benchmarks to be compiled but is not a benchmark. I made a patch in the following commit (https://github.com/Crypt-iQ/bitcoin/commit/be07dbc6f26638f7ec187e42144ccc5dab90c0a7) that reduced th
...
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268239265)
I think it must have been a fluke because when I synced up to a little past block ~200K, `fwrite` was in the lead and `ftell` and xor were about tied at ~3% each of the total time spent. I wrote a test here https://github.com/Crypt-iQ/bitcoin/commit/bf924d6d2d4020e799796c0751ac64c892ae8e6b that requires the benchmarks to be compiled but is not a benchmark. I made a patch in the following commit (https://github.com/Crypt-iQ/bitcoin/commit/be07dbc6f26638f7ec187e42144ccc5dab90c0a7) that reduced th
...
💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268245361)
I think the calls to `ftell` in `WriteBlockToDisk` and `UndoWriteToDisk` can be removed since the file position is known before the call and the number of bytes written is also known in the success case (in the failure case the writes will throw), so it should just be possible to increment the initial file position used in `OpenBlockFile` / `OpenUndoFile` ?
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268245361)
I think the calls to `ftell` in `WriteBlockToDisk` and `UndoWriteToDisk` can be removed since the file position is known before the call and the number of bytes written is also known in the success case (in the failure case the writes will throw), so it should just be possible to increment the initial file position used in `OpenBlockFile` / `OpenUndoFile` ?
💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1268252521)
I am still not convinced about `auto` and brace-initialization.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax mentions:
> Use = only when you are sure that there can be no narrowing conversions
> For built-in arithmetic types, use = only with auto
There will not be narrowing conversions with `auto x = expression;`
One of the examples down there uses `auto p = ...`.
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1268252521)
I am still not convinced about `auto` and brace-initialization.
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es23-prefer-the--initializer-syntax mentions:
> Use = only when you are sure that there can be no narrowing conversions
> For built-in arithmetic types, use = only with auto
There will not be narrowing conversions with `auto x = expression;`
One of the examples down there uses `auto p = ...`.
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268252629)
I guess it depends on the data written. My benchmark was for 1MB written. With 33 bytes written, `ftell` is the dominant:
```
ROUTINE ====================== AutoFile::write in src/streams.cpp
2 28 Total samples (flat / cumulative)
. . 40: nSize -= nNow;
. . 41: }
. . 42: }
. . 43:
. . 44: void AutoFile::write(Span<const std::byte> src)
---
. . 45: {
. . 46: if (!m_file) th
...
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268252629)
I guess it depends on the data written. My benchmark was for 1MB written. With 33 bytes written, `ftell` is the dominant:
```
ROUTINE ====================== AutoFile::write in src/streams.cpp
2 28 Total samples (flat / cumulative)
. . 40: nSize -= nNow;
. . 41: }
. . 42: }
. . 43:
. . 44: void AutoFile::write(Span<const std::byte> src)
---
. . 45: {
. . 46: if (!m_file) th
...
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268255975)
I mean I am happy to make the change, but it seems brittle if someone calls `std::fseek` after the constructor?
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268255975)
I mean I am happy to make the change, but it seems brittle if someone calls `std::fseek` after the constructor?
💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268272983)
Hm good point, I didn't think about that -- it does seem like those functions are pretty contained since they open the file at the start and then close it at exit, but it does seem brittle
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268272983)
Hm good point, I didn't think about that -- it does seem like those functions are pretty contained since they open the file at the start and then close it at exit, but it does seem brittle
💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268286275)
In any case the bottleneck doesn't seem to be `std::array<std::byte, 4096> buf;`, so I think this thread can be closed?
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268286275)
In any case the bottleneck doesn't seem to be `std::array<std::byte, 4096> buf;`, so I think this thread can be closed?
💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268291011)
yeah I didn't see allocations come up at all in perf
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268291011)
yeah I didn't see allocations come up at all in perf