:lock: fanquake locked an issue: "Bitcoin-core. MacOs"
(https://github.com/bitcoin/bitcoin/issues/27193)
(https://github.com/bitcoin/bitcoin/issues/27193)
💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1490015098)
Since the `replySent` parameter of the `HTTPRequest` constructor is used only from the tests, I would suggest to remove it and adjust the test as needed. I believe the tests should be testing the same API that is used by the real code, otherwise they may end up testing scenarios that never happen in real code and missing real code scenarios.
Furthermore that `replySent = true` from the test is a hack to workaround another problem. It is actually a lie - the reply has not been sent. The proble
...
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1490015098)
Since the `replySent` parameter of the `HTTPRequest` constructor is used only from the tests, I would suggest to remove it and adjust the test as needed. I believe the tests should be testing the same API that is used by the real code, otherwise they may end up testing scenarios that never happen in real code and missing real code scenarios.
Furthermore that `replySent = true` from the test is a hack to workaround another problem. It is actually a lie - the reply has not been sent. The proble
...
📝 hebasto opened a pull request: "refactor: Drop no longer used `CNetMsgMaker` instances"
(https://github.com/bitcoin/bitcoin/pull/27368)
The removed lines have been unused since the https://github.com/bitcoin/bitcoin/commit/abf5d16c24cb08b0451bdbd4d1de63a12930e8f5 commit from https://github.com/bitcoin/bitcoin/pull/25454.
(https://github.com/bitcoin/bitcoin/pull/27368)
The removed lines have been unused since the https://github.com/bitcoin/bitcoin/commit/abf5d16c24cb08b0451bdbd4d1de63a12930e8f5 commit from https://github.com/bitcoin/bitcoin/pull/25454.
💬 Sjors commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1153050139)
I'm guessing that just dropping the space will fix the CI issue?
```cpp
/*fRequestedInternal=*/false
```
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1153050139)
I'm guessing that just dropping the space will fix the CI issue?
```cpp
/*fRequestedInternal=*/false
```
💬 Sjors commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1490063128)
> bare OP_RETURN outputs are not data carrying outputs
Agreed. But if you stick to this approach, please write a test.
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1490063128)
> bare OP_RETURN outputs are not data carrying outputs
Agreed. But if you stick to this approach, please write a test.
💬 dergoegge commented on pull request "ci: use LLVM/clang-16 in native_fuzz (ASAN) job":
(https://github.com/bitcoin/bitcoin/pull/27363#issuecomment-1490075350)
utACK a634c288c34627eb4ea6f919c99339256f490ede
(https://github.com/bitcoin/bitcoin/pull/27363#issuecomment-1490075350)
utACK a634c288c34627eb4ea6f919c99339256f490ede
👍 dergoegge approved a pull request: "ci: use LLVM/clang-16 in native_fuzz (ASAN) job"
(https://github.com/bitcoin/bitcoin/pull/27363)
(https://github.com/bitcoin/bitcoin/pull/27363)
💬 darosior commented on issue "`listtransactions` fails to list self-send transactions (for imported descriptor wallet)":
(https://github.com/bitcoin/bitcoin/issues/26096#issuecomment-1490083897)
@msgilligan any luck with your testing?
(https://github.com/bitcoin/bitcoin/issues/26096#issuecomment-1490083897)
@msgilligan any luck with your testing?
💬 Bushstar commented on pull request "refactor: remove unused param from legacy pubkey interface":
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1153075085)
CI is passing now after rename `internal` to `fRequestedInternal`. CI checks below appear all green to me.
(https://github.com/bitcoin/bitcoin/pull/27274#discussion_r1153075085)
CI is passing now after rename `internal` to `fRequestedInternal`. CI checks below appear all green to me.
💬 dergoegge commented on pull request "refactor: Drop no longer used `CNetMsgMaker` instances":
(https://github.com/bitcoin/bitcoin/pull/27368#issuecomment-1490101331)
utACK ea7ec7808745805c0a18513d7da271dedb2de3f1
(https://github.com/bitcoin/bitcoin/pull/27368#issuecomment-1490101331)
utACK ea7ec7808745805c0a18513d7da271dedb2de3f1
💬 hebasto commented on pull request "p2p: Avoid multiple getheaders messages in flight to the same peer":
(https://github.com/bitcoin/bitcoin/pull/25454#issuecomment-1490109063)
> `msgMaker` is now unused in ProcessHeadersMessage.
Same for `PeerManagerImpl::HandleFewUnconnectingHeaders()` and `PeerManagerImpl::ConsiderEviction()`.
See https://github.com/bitcoin/bitcoin/pull/27368.
(https://github.com/bitcoin/bitcoin/pull/25454#issuecomment-1490109063)
> `msgMaker` is now unused in ProcessHeadersMessage.
Same for `PeerManagerImpl::HandleFewUnconnectingHeaders()` and `PeerManagerImpl::ConsiderEviction()`.
See https://github.com/bitcoin/bitcoin/pull/27368.
💬 hebasto commented on pull request "guix: use python-minimal (3.9)":
(https://github.com/bitcoin/bitcoin/pull/27361#issuecomment-1490111066)
Guix builds:
```
53aa12b7fe92d92eaf81a51a8f5d534f92a2f18f0218f29fdf3221d817a53c04 guix-build-d0e571ebb187/output/aarch64-linux-gnu/SHA256SUMS.part
01467530b06b90114561e86968710250e3648fe4aba5c228781ccf9ac72394c2 guix-build-d0e571ebb187/output/aarch64-linux-gnu/bitcoin-d0e571ebb187-aarch64-linux-gnu-debug.tar.gz
fdf6dfc901e371b4e11a7f69eb2ff9b36a3ac393255862bad1dcd45b54186dca guix-build-d0e571ebb187/output/aarch64-linux-gnu/bitcoin-d0e571ebb187-aarch64-linux-gnu.tar.gz
6345b0f6b6156d84b24
...
(https://github.com/bitcoin/bitcoin/pull/27361#issuecomment-1490111066)
Guix builds:
```
53aa12b7fe92d92eaf81a51a8f5d534f92a2f18f0218f29fdf3221d817a53c04 guix-build-d0e571ebb187/output/aarch64-linux-gnu/SHA256SUMS.part
01467530b06b90114561e86968710250e3648fe4aba5c228781ccf9ac72394c2 guix-build-d0e571ebb187/output/aarch64-linux-gnu/bitcoin-d0e571ebb187-aarch64-linux-gnu-debug.tar.gz
fdf6dfc901e371b4e11a7f69eb2ff9b36a3ac393255862bad1dcd45b54186dca guix-build-d0e571ebb187/output/aarch64-linux-gnu/bitcoin-d0e571ebb187-aarch64-linux-gnu.tar.gz
6345b0f6b6156d84b24
...
👍 hebasto approved a pull request: "guix: use python-minimal (3.9)"
(https://github.com/bitcoin/bitcoin/pull/27361)
ACK d0e571ebb187d7c4c2821f1334cb2dd4222dd8ce
(https://github.com/bitcoin/bitcoin/pull/27361)
ACK d0e571ebb187d7c4c2821f1334cb2dd4222dd8ce
👍 TheCharlatan approved a pull request: "refactor: Drop no longer used `CNetMsgMaker` instances"
(https://github.com/bitcoin/bitcoin/pull/27368)
ACK ea7ec7808745805c0a18513d7da271dedb2de3f1
(https://github.com/bitcoin/bitcoin/pull/27368)
ACK ea7ec7808745805c0a18513d7da271dedb2de3f1
💬 vasild commented on pull request "addrman: treat Tor/I2P/CJDNS as a single group":
(https://github.com/bitcoin/bitcoin/pull/22563#issuecomment-1490127136)
Yes. But I think I should not give up on treating Tor sources as one source because this is what it is in reality. And then, addrman should be able to accommodate 10s of thousands of addresses from one source (if the source is Tor, I2P or CJDNS) and still not allow a single malicious entity to flood it.
(https://github.com/bitcoin/bitcoin/pull/22563#issuecomment-1490127136)
Yes. But I think I should not give up on treating Tor sources as one source because this is what it is in reality. And then, addrman should be able to accommodate 10s of thousands of addresses from one source (if the source is Tor, I2P or CJDNS) and still not allow a single malicious entity to flood it.
💬 glozow commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153118703)
Question: what is the reason to put `m_recently_announced_invs` in `Peer` as opposed to `TxRelay` within `Peer`?
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153118703)
Question: what is the reason to put `m_recently_announced_invs` in `Peer` as opposed to `TxRelay` within `Peer`?
⚠️ vasild opened an issue: "Fix net grouping of non-IP networks"
(https://github.com/bitcoin/bitcoin/issues/27369)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
`NetGroupManager::GetGroup()` was designed with IP networks in mind, where for example `10.20.30.40` and `10.20.30.45` are "close" and likely to belong to the same entity, while `100.20.30.40` is "distant" from them. This does not apply however for Tor, I2P and CJDNS where the address is "random" bytes. Actually, Tor, I2P and CJDNS addresses that have common prefix are _harder_ to obtain t
...
(https://github.com/bitcoin/bitcoin/issues/27369)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
`NetGroupManager::GetGroup()` was designed with IP networks in mind, where for example `10.20.30.40` and `10.20.30.45` are "close" and likely to belong to the same entity, while `100.20.30.40` is "distant" from them. This does not apply however for Tor, I2P and CJDNS where the address is "random" bytes. Actually, Tor, I2P and CJDNS addresses that have common prefix are _harder_ to obtain t
...
💬 vasild commented on pull request "p2p: Improve diversification of new connections":
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1490154556)
How? I opened https://github.com/bitcoin/bitcoin/issues/27369 to discuss ideas on how to approach this.
(https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1490154556)
How? I opened https://github.com/bitcoin/bitcoin/issues/27369 to discuss ideas on how to approach this.
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1153139259)
https://cirrus-ci.com/task/4526522288046080
seems like the tidy job wants you to add this:
```suggestion
std::vector<uint256> txids_needed;
txids_needed.reserve(m_requested_outpoints_by_txid.size());
```
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1153139259)
https://cirrus-ci.com/task/4526522288046080
seems like the tidy job wants you to add this:
```suggestion
std::vector<uint256> txids_needed;
txids_needed.reserve(m_requested_outpoints_by_txid.size());
```
💬 stratospher commented on issue "Fix net grouping of non-IP networks":
(https://github.com/bitcoin/bitcoin/issues/27369#issuecomment-1490173773)
https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1490154556 - workaround it by not inserting tor/i2p/cjdns address in `setConnected` for now? so `GetGroup` wouldn't be called on these networks.
(https://github.com/bitcoin/bitcoin/issues/27369#issuecomment-1490173773)
https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1490154556 - workaround it by not inserting tor/i2p/cjdns address in `setConnected` for now? so `GetGroup` wouldn't be called on these networks.