💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261645151)
commit 543273d96e896adf5531ed961856aa0eb70cbe57:
nit: `uint256&` like in the line above?
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261645151)
commit 543273d96e896adf5531ed961856aa0eb70cbe57:
nit: `uint256&` like in the line above?
💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261605505)
There should be a `setmocktime` call with the initial time somewhere before the first `fastforward`, otherwise this can fail intermittently.
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261605505)
There should be a `setmocktime` call with the initial time somewhere before the first `fastforward`, otherwise this can fail intermittently.
💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261668254)
4cbb63175398d9b8afe755d2adb24edbd2b4913b:
nit: could use `orphan_wtxid`, here and in various other spots.
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261668254)
4cbb63175398d9b8afe755d2adb24edbd2b4913b:
nit: could use `orphan_wtxid`, here and in various other spots.
💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261779747)
I don't think there should ever be a `\n` prefix, why would we want to separate the meta information (timestamp, threadname etc.) from the actual log entry?
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261779747)
I don't think there should ever be a `\n` prefix, why would we want to separate the meta information (timestamp, threadname etc.) from the actual log entry?
💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261690923)
29d9d326d5193bb9a410a8881eabc93de5dd6266:
`EraseOrphanOfPeer` looks like a rebase error (it's never implemented and removed in d92b017f6818c1de3286e5dbc35af3860fdf7547).
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261690923)
29d9d326d5193bb9a410a8881eabc93de5dd6266:
`EraseOrphanOfPeer` looks like a rebase error (it's never implemented and removed in d92b017f6818c1de3286e5dbc35af3860fdf7547).
💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261786477)
nit: could use `orphan_wtxid` introduced in the previous commit, here and in various other places.
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261786477)
nit: could use `orphan_wtxid` introduced in the previous commit, here and in various other places.
💬 achow101 commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261786736)
In 7601a60cddc4405f58f71879f996322329509dfe "crypto: add Poly1305 class with std::byte Span interface"
Use `UCharCast` cast here (and below)?
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261786736)
In 7601a60cddc4405f58f71879f996322329509dfe "crypto: add Poly1305 class with std::byte Span interface"
Use `UCharCast` cast here (and below)?
💬 theStack commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261813389)
```suggestion
std::vector<std::byte> key(32, std::byte{(unsigned char)i});
std::vector<std::byte> msg(i, std::byte{(unsigned char)i});
```
With the same idea as above. I think all `Make{Writable}ByteSpan` calls can be avoided in this commit, which improves readability IMHO quite a bit.
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261813389)
```suggestion
std::vector<std::byte> key(32, std::byte{(unsigned char)i});
std::vector<std::byte> msg(i, std::byte{(unsigned char)i});
```
With the same idea as above. I think all `Make{Writable}ByteSpan` calls can be avoided in this commit, which improves readability IMHO quite a bit.
🤔 theStack reviewed a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993#pullrequestreview-1527358127)
LGTM overall. Verified the introduced test vectors (40de8ce39a65cb37a74bebcbf7b34ed8e70b7096) with the [pyca/cryptography](https://cryptography.io/en/latest/) library, which uses OpenSSL in the background (tried with PyCryptodome first, but that didn't allow use of raw Poly1305 without a cipher algorithm). As expected, everything passes with this alternative implementation as well.
<details>
<summary>Python code</summary>
```python
#!/usr/bin/env python3
from cryptography.hazmat.primitive
...
(https://github.com/bitcoin/bitcoin/pull/27993#pullrequestreview-1527358127)
LGTM overall. Verified the introduced test vectors (40de8ce39a65cb37a74bebcbf7b34ed8e70b7096) with the [pyca/cryptography](https://cryptography.io/en/latest/) library, which uses OpenSSL in the background (tried with PyCryptodome first, but that didn't allow use of raw Poly1305 without a cipher algorithm). As expected, everything passes with this alternative implementation as well.
<details>
<summary>Python code</summary>
```python
#!/usr/bin/env python3
from cryptography.hazmat.primitive
...
💬 theStack commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261809644)
could let `ParseHex` return a `std::vector<std::byte>` here and below (for the `final_tag` comparison) to avoid `MakeByteSpan` calls later
```suggestion
auto total_key = ParseHex<std::byte>("01020304050607fffefdfcfbfaf9ffffffffffffffffffffffffffff00000000");
```
(maybe at some point in the future we can change `ParseHex` to return a vector of `std::byte` rather than `unsigned char` by default)
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261809644)
could let `ParseHex` return a `std::vector<std::byte>` here and below (for the `final_tag` comparison) to avoid `MakeByteSpan` calls later
```suggestion
auto total_key = ParseHex<std::byte>("01020304050607fffefdfcfbfaf9ffffffffffffffffffffffffffff00000000");
```
(maybe at some point in the future we can change `ParseHex` to return a vector of `std::byte` rather than `unsigned char` by default)
💬 megumin9 commented on issue "The destructor of CCheckQueueControl may throw a exception ":
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1633404649)
I understand and I'm sorry to bother you.
(https://github.com/bitcoin/bitcoin/issues/28033#issuecomment-1633404649)
I understand and I'm sorry to bother you.
💬 ajtowns commented on issue "Auto detect IPv6 connectivity":
(https://github.com/bitcoin/bitcoin/issues/28061#issuecomment-1633414939)
> is `EADDRNOTAVAIL` then that means the machine has no IPv6 address configured (but it may be configured later).
If the system can be reconfigured at runtime to make ipv6 available, it seems a bad idea to force the user to also have to restart bitcoind in order for bitcoind to start making/accepting ipv6 connections. (You might also have ipv6 fail due to your router/isp not supporting ipv6, which also may change at runtime)
(https://github.com/bitcoin/bitcoin/issues/28061#issuecomment-1633414939)
> is `EADDRNOTAVAIL` then that means the machine has no IPv6 address configured (but it may be configured later).
If the system can be reconfigured at runtime to make ipv6 available, it seems a bad idea to force the user to also have to restart bitcoind in order for bitcoind to start making/accepting ipv6 connections. (You might also have ipv6 fail due to your router/isp not supporting ipv6, which also may change at runtime)
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1261909269)
Maybe we could recover that as an invariant by having txs added in `MaybeUpdateMempoolForReorg` set the sequence number as 0? The idea being that if they've made it into a block then we've probably announced the headers already so it's no secret that we knew about the tx anyway...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1261909269)
Maybe we could recover that as an invariant by having txs added in `MaybeUpdateMempoolForReorg` set the sequence number as 0? The idea being that if they've made it into a block then we've probably announced the headers already so it's no secret that we knew about the tx anyway...
💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914712)
Done.
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914712)
Done.
💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914858)
Oh nice, I wasn't aware of `ParseHex<std::byte>`. Done.
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914858)
Oh nice, I wasn't aware of `ParseHex<std::byte>`. Done.
💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914931)
Done.
(https://github.com/bitcoin/bitcoin/pull/27993#discussion_r1261914931)
Done.
💬 MarcoFalke commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1633606181)
> Why not CXXFLAGS='$CXXFLAGS -gdwarf-4'?
Feel free to test it and make an alternative pull, but I am pretty sure it will not work. Also, I don't see any upside and it is more verbose.
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1633606181)
> Why not CXXFLAGS='$CXXFLAGS -gdwarf-4'?
Feel free to test it and make an alternative pull, but I am pretty sure it will not work. Also, I don't see any upside and it is more verbose.
💬 MarcoFalke commented on pull request "RPC: Add universal options argument to listtransactions":
(https://github.com/bitcoin/bitcoin/pull/22807#discussion_r1262056637)
```
wallet/rpc/transactions.cpp:441:75: error: no member named 'OMITTED_NAMED_ARG' in 'RPCArg::Optional'
{"options|skip", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "for backward compatibility: passing in a number instead of an object will result in {\"skip\":nnn}",
~~~~~~~~~~~~~~~~~~^
wallet/rpc/transactions.cpp:508:9: error: use of undeclared identifier 'RPCTypeCheckArgument'
RPCTypeCheckArgum
...
(https://github.com/bitcoin/bitcoin/pull/22807#discussion_r1262056637)
```
wallet/rpc/transactions.cpp:441:75: error: no member named 'OMITTED_NAMED_ARG' in 'RPCArg::Optional'
{"options|skip", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "for backward compatibility: passing in a number instead of an object will result in {\"skip\":nnn}",
~~~~~~~~~~~~~~~~~~^
wallet/rpc/transactions.cpp:508:9: error: use of undeclared identifier 'RPCTypeCheckArgument'
RPCTypeCheckArgum
...
💬 glozow commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1262316909)
Yeah good point.
It seems that, given the need to synchronize between `TxRequestTracker` and the package tracking stuff, we should have them both guarded by 1 lock. Looking at #26151, it seems like we could have a `m_txrequest GUARDED_BY(tx_download_mutex)`, `m_txpackagetracker GUARDED_BY(tx_download_mutex)`, and lock it from these peerman functions?
Alternatively, I wonder if it makes sense to take it a step further and put this all in a `TxDownloadManager` module that wraps orphanage, tx
...
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1262316909)
Yeah good point.
It seems that, given the need to synchronize between `TxRequestTracker` and the package tracking stuff, we should have them both guarded by 1 lock. Looking at #26151, it seems like we could have a `m_txrequest GUARDED_BY(tx_download_mutex)`, `m_txpackagetracker GUARDED_BY(tx_download_mutex)`, and lock it from these peerman functions?
Alternatively, I wonder if it makes sense to take it a step further and put this all in a `TxDownloadManager` module that wraps orphanage, tx
...
👍 MarcoFalke approved a pull request: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1528038830)
lgtm ACK fe9da891eb7a7440b6c23d63e129525fe26f8f0e 🚊
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK fe9da891eb7a7440
...
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1528038830)
lgtm ACK fe9da891eb7a7440b6c23d63e129525fe26f8f0e 🚊
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK fe9da891eb7a7440
...