💬 fanquake commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1642153266)
> So I was wondering if the time machine could be bumped (for mingw), but the Linux tasks be kept as-is for now?
We don't currently have an easy way to use separate time-machines for differents HOSTS, and I'm not sure if that's something we'd want to do. The general time-machine bump is only blocked on one last issue, so that should be done shortly.
> (I mean, obviously my preference would be to use clang for Windows cross-builds,
I wouldn't be opposed to that. Although note that in tha
...
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1642153266)
> So I was wondering if the time machine could be bumped (for mingw), but the Linux tasks be kept as-is for now?
We don't currently have an easy way to use separate time-machines for differents HOSTS, and I'm not sure if that's something we'd want to do. The general time-machine bump is only blocked on one last issue, so that should be done shortly.
> (I mean, obviously my preference would be to use clang for Windows cross-builds,
I wouldn't be opposed to that. Although note that in tha
...
📝 dergoegge opened a pull request: "rfc: Type-safe transaction identifiers"
(https://github.com/bitcoin/bitcoin/pull/28107)
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.
This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.
(Only t
...
(https://github.com/bitcoin/bitcoin/pull/28107)
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.
This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.
(Only t
...
💬 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
...