💬 MarcoFalke commented on pull request "test: Add missing `set -ex` to ci/lint/06_script.sh":
(https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268140307)
A fail-fast setting would also need to be picked up by lint-all to abort early, because this one does collect the failures.
But I won't be working on that soon, so should I just close this pull?
(https://github.com/bitcoin/bitcoin/pull/28103#discussion_r1268140307)
A fail-fast setting would also need to be picked up by lint-all to abort early, because this one does collect the failures.
But I won't be working on that soon, so should I just close this pull?
💬 MarcoFalke commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1642175486)
How is this different from `GenTxid`?
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1642175486)
How is this different from `GenTxid`?
💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1268148641)
We have already checked if UNIX sockets are supported.
```cpp
#if HAVE_SOCKADDR_UN
argsman.AddArg("-onion=<ip:port|path>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy). May be a local file path prefixed with 'unix:' if UNIX domain sockets are supported by the platform.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
#else
argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion
...
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1268148641)
We have already checked if UNIX sockets are supported.
```cpp
#if HAVE_SOCKADDR_UN
argsman.AddArg("-onion=<ip:port|path>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy). May be a local file path prefixed with 'unix:' if UNIX domain sockets are supported by the platform.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
#else
argsman.AddArg("-onion=<ip:port>", "Use separate SOCKS5 proxy to reach peers via Tor onion
...
💬 dergoegge commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1642196812)
> How is this different from GenTxid?
I think mostly compile-time vs. runtime checks? `GenTxid` can hold both txid types but doesn't enforce type correctness at compile time. Any type checks would happen at runtime, e.g. `assert(gtxid.IsWtxid())`. Also nothing prevents passing the wrong txid type when constructing a `GenTxid`.
(I guess with this PR we could consider making `GenTxid` a `std::variant<Txid, Wtxid>`)
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1642196812)
> How is this different from GenTxid?
I think mostly compile-time vs. runtime checks? `GenTxid` can hold both txid types but doesn't enforce type correctness at compile time. Any type checks would happen at runtime, e.g. `assert(gtxid.IsWtxid())`. Also nothing prevents passing the wrong txid type when constructing a `GenTxid`.
(I guess with this PR we could consider making `GenTxid` a `std::variant<Txid, Wtxid>`)
💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268176279)
Boy I was really scratching my head over this for like an hour ("how does this work at all?") then I remember the genesis block is written to a file when block count is still 0. So that's how we are rounding up.
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1268176279)
Boy I was really scratching my head over this for like an hour ("how does this work at all?") then I remember the genesis block is written to a file when block count is still 0. So that's how we are rounding up.
💬 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 = ...`.