💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253707986)
That's moving a member of a local struct/object, not a simple local, so it can't have been constructed in place and copy/move elision isn't possible, though?
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253707986)
That's moving a member of a local struct/object, not a simple local, so it can't have been constructed in place and copy/move elision isn't possible, though?
💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253709698)
Ah, you may be right.
https://shaharmike.com/cpp/rvo/#returning-member
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253709698)
Ah, you may be right.
https://shaharmike.com/cpp/rvo/#returning-member
💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253739563)
<details><summary>If this test is valid (arm64 clang 16), returning the struct member without std::move may be better</summary><p>
```cpp
// rvo.cpp
#include <iostream>
struct Snitch { // Note: All methods have side effects
Snitch() { std::cout << "ctor" << std::endl; }
~Snitch() { std::cout << "dtor" << std::endl; }
Snitch(const Snitch&) { std::cout << "copy ctor" << std::endl; }
Snitch(Snitch&&) { std::cout << "move ctor" << std::endl; }
Snitch& operato
...
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253739563)
<details><summary>If this test is valid (arm64 clang 16), returning the struct member without std::move may be better</summary><p>
```cpp
// rvo.cpp
#include <iostream>
struct Snitch { // Note: All methods have side effects
Snitch() { std::cout << "ctor" << std::endl; }
~Snitch() { std::cout << "dtor" << std::endl; }
Snitch(const Snitch&) { std::cout << "copy ctor" << std::endl; }
Snitch(Snitch&&) { std::cout << "move ctor" << std::endl; }
Snitch& operato
...
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253743213)
For wtxid relay peers the prior `.insert(hash)` line adds the wtxid to the filter; for non-wtxid relay peers, it adds the txid to the filter. We only need to do a second insert to add the txid for wtxid relay peers, and then only when the wtxid is actually different to the txid. The extra check is just because testing a bool is easier than comparing two uint256s.
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253743213)
For wtxid relay peers the prior `.insert(hash)` line adds the wtxid to the filter; for non-wtxid relay peers, it adds the txid to the filter. We only need to do a second insert to add the txid for wtxid relay peers, and then only when the wtxid is actually different to the txid. The extra check is just because testing a bool is easier than comparing two uint256s.
📝 achow101 reopened a pull request: "RPC/Wallet: Add "use_txids" to output of getaddressinfo"
(https://github.com/bitcoin/bitcoin/pull/22693)
(Non-GUI part of #15987, but without the bloom stuff)
(https://github.com/bitcoin/bitcoin/pull/22693)
(Non-GUI part of #15987, but without the bloom stuff)
💬 luke-jr commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#discussion_r1253745180)
That would break the wallettool again
(https://github.com/bitcoin/bitcoin/pull/22693#discussion_r1253745180)
That would break the wallettool again
💬 luke-jr commented on pull request "RPC/Wallet: Add "use_txids" to output of getaddressinfo":
(https://github.com/bitcoin/bitcoin/pull/22693#issuecomment-1622665055)
Rebased.
>if I understand correctly, this PR only makes it easier to tell if you're reusing one of your own wallet addresses for receiving, it doesn't tell you if you're reusing an address you've already used to send to someone else.
> do you want to have the "use_txids" for all addresses throughout the whole blockchain or only for a subset of them?
As documented, it only keeps track of addresses used in wallet transactions (whether we are the sender or receiver).
(https://github.com/bitcoin/bitcoin/pull/22693#issuecomment-1622665055)
Rebased.
>if I understand correctly, this PR only makes it easier to tell if you're reusing one of your own wallet addresses for receiving, it doesn't tell you if you're reusing an address you've already used to send to someone else.
> do you want to have the "use_txids" for all addresses throughout the whole blockchain or only for a subset of them?
As documented, it only keeps track of addresses used in wallet transactions (whether we are the sender or receiver).
👋 luke-jr's pull request is ready for review: "RPC/Wallet: Add "use_txids" to output of getaddressinfo"
(https://github.com/bitcoin/bitcoin/pull/22693)
(https://github.com/bitcoin/bitcoin/pull/22693)
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253748543)
Updated
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253748543)
Updated
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253751283)
I think the test you want is `auto s = Wrapper(); return std::move(s.snitch);`
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253751283)
I think the test you want is `auto s = Wrapper(); return std::move(s.snitch);`
💬 jonatack commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253753580)
You're right, thanks.
```
foo_move...
ctor
move ctor
dtor
foo...
ctor
copy ctor
dtor
```
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1253753580)
You're right, thanks.
```
foo_move...
ctor
move ctor
dtor
foo...
ctor
copy ctor
dtor
```
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1622681555)
Updated for jonatack's review -- removes unneeded code changes, updates comments.
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1622681555)
Updated for jonatack's review -- removes unneeded code changes, updates comments.
🤔 theStack reviewed a pull request: "Add support for RFC8439 variant of ChaCha20"
(https://github.com/bitcoin/bitcoin/pull/27985#pullrequestreview-1515520041)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27985#pullrequestreview-1515520041)
Concept ACK
💬 theStack commented on pull request "Add support for RFC8439 variant of ChaCha20":
(https://github.com/bitcoin/bitcoin/pull/27985#discussion_r1253759518)
tiny nit (probably only worth changing if you have to retouch): could check this assert unconditionally, to not allow test cases where `hex_input` is empty but `hex_expected_output` is not
(https://github.com/bitcoin/bitcoin/pull/27985#discussion_r1253759518)
tiny nit (probably only worth changing if you have to retouch): could check this assert unconditionally, to not allow test cases where `hex_input` is empty but `hex_expected_output` is not
💬 theStack commented on pull request "test: refactor: deduplicate legacy ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28025#discussion_r1253771758)
Seems like the input index is checked in `LegacySignatureMsg` (which is in turn called by `LegacySignatureHash`): https://github.com/bitcoin/bitcoin/blob/bc4f6b13feb29146b7e10e86f93dc7f6fb6937f2/test/functional/test_framework/script.py#L638-L639
For `sighash_type` it might be nice to add a type annotation, though I don't know if the byte range (0..255) can be specified exactly (currently that value is not set explicitly, as all our call-sites use the default SIGHASH_ALL). As for `tx.vin[input_i
...
(https://github.com/bitcoin/bitcoin/pull/28025#discussion_r1253771758)
Seems like the input index is checked in `LegacySignatureMsg` (which is in turn called by `LegacySignatureHash`): https://github.com/bitcoin/bitcoin/blob/bc4f6b13feb29146b7e10e86f93dc7f6fb6937f2/test/functional/test_framework/script.py#L638-L639
For `sighash_type` it might be nice to add a type annotation, though I don't know if the byte range (0..255) can be specified exactly (currently that value is not set explicitly, as all our call-sites use the default SIGHASH_ALL). As for `tx.vin[input_i
...
💬 theStack commented on pull request "test: refactor: deduplicate legacy ECDSA signing for tx inputs":
(https://github.com/bitcoin/bitcoin/pull/28025#issuecomment-1622720582)
> Also - do you think we need a segwitV0 function like this as well? As far as I can grep there's only 2 such signing operations in `p2p_segwit.py` so maybe that's more work than its worth.
Good point -- I think it would be nice to introduce similar functions for other signature types (e.g. `sign_input_segwitv0`) for consistency reasons. Might be worth a follow-up, together with some other improvements pointed out in https://github.com/bitcoin/bitcoin/pull/28025#discussion_r1253580835 (type a
...
(https://github.com/bitcoin/bitcoin/pull/28025#issuecomment-1622720582)
> Also - do you think we need a segwitV0 function like this as well? As far as I can grep there's only 2 such signing operations in `p2p_segwit.py` so maybe that's more work than its worth.
Good point -- I think it would be nice to introduce similar functions for other signature types (e.g. `sign_input_segwitv0`) for consistency reasons. Might be worth a follow-up, together with some other improvements pointed out in https://github.com/bitcoin/bitcoin/pull/28025#discussion_r1253580835 (type a
...
💬 ajtowns commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1253796131)
This implementation of that logic doesn't seem ideal to me:
* sending the TX without doing INV/GETDATA first risks having the TX be ignored as unrequested
* `PushUnbroadcastTx` doesn't actually send the TX, but `MaybeSendPing` does actually send the PING, so you might get the PONG and closes the connection before the other end has actually received/processed the TX
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1253796131)
This implementation of that logic doesn't seem ideal to me:
* sending the TX without doing INV/GETDATA first risks having the TX be ignored as unrequested
* `PushUnbroadcastTx` doesn't actually send the TX, but `MaybeSendPing` does actually send the PING, so you might get the PONG and closes the connection before the other end has actually received/processed the TX
💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253806859)
CI failing at this line.
https://cirrus-ci.com/task/5531648075235328?logs=ci#L3397
https://cirrus-ci.com/task/5531648075235328?logs=ci#L4424
`Expected messages "['DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat']" does not partially match log:`
```
- 2023-07-05T18:23:01.237732Z [shutoff] [logging/timer.h:56] [Log] DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat started
- 2023-07-05T18:23:01.286842Z [shutoff] [logging/timer.h
...
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253806859)
CI failing at this line.
https://cirrus-ci.com/task/5531648075235328?logs=ci#L3397
https://cirrus-ci.com/task/5531648075235328?logs=ci#L4424
`Expected messages "['DumpAnchors: Flush 1 outbound block-relay-only peer addresses to anchors.dat']" does not partially match log:`
```
- 2023-07-05T18:23:01.237732Z [shutoff] [logging/timer.h:56] [Log] DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat started
- 2023-07-05T18:23:01.286842Z [shutoff] [logging/timer.h
...
💬 jonatack commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253818208)
Reproduced this test failure locally on the 4th run of `test/functional/feature_anchors.py` (arm64 macOS 13.4.1)
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1253818208)
Reproduced this test failure locally on the 4th run of `test/functional/feature_anchors.py` (arm64 macOS 13.4.1)
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1253827052)
We already require `NET_*` to go from 0 to NET_MAX with no gaps every time we do `for (int n = 0; n < NET_MAX; ++n)`. It's a standard expectation of enums and it's documented as such in netaddress.h. It's not an added requirement, and doesn't make the code fragile or harder to change...
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1253827052)
We already require `NET_*` to go from 0 to NET_MAX with no gaps every time we do `for (int n = 0; n < NET_MAX; ++n)`. It's a standard expectation of enums and it's documented as such in netaddress.h. It's not an added requirement, and doesn't make the code fragile or harder to change...