💬 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.

💬 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
```

💬 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;
}
```

💬 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.

💬 dergoegge commented on pull request "net processing, refactor: Decouple PeerManager from gArgs":
(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
...

💬 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.

💬 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
...

💬 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` ?

💬 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 = ...`.

💬 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
...

💬 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?

💬 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

💬 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?

💬 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

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268305587)
Maybe I'll pick up https://github.com/bitcoin/bitcoin/pull/28006 again and remove `Get`?

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1642386101)
> needs rebase if still relevant.

For the `previous releases` CI? (I don't find any merge conflicts with current master.)

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1642398008)
Pushed a small change to call `std::ftell` less often if a large data span is written. In a follow-up, changes can be made to call it less often when small data spans are written.

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1642400630)
> I don't find any merge conflicts with current master.

Are you sure you ran a full build after the merge?

💬 Crypt-iQ commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#discussion_r1268324286)
happy to review if you do