💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443334583)
Fair enough. The check doesn't hurt though, and perhaps functions as documentation to clarify the conditions due to which these tests are possible?
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443334583)
Fair enough. The check doesn't hurt though, and perhaps functions as documentation to clarify the conditions due to which these tests are possible?
💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443336121)
Yes, I agree - I think that there are colliding goals, my thinking is as follows:
1) The `m_protect` protection attempts to prevent pathological situations where we wouldn't have peers with the honest chain (that shouldn't happen normally even without the protection, but would be very serious if they did)
2) The network-specific protection aims to be connected to all networks we support, to help the interconnection, and also to reduce the risk of eclipse attacks. Ideally, that means relaying b
...
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443336121)
Yes, I agree - I think that there are colliding goals, my thinking is as follows:
1) The `m_protect` protection attempts to prevent pathological situations where we wouldn't have peers with the honest chain (that shouldn't happen normally even without the protection, but would be very serious if they did)
2) The network-specific protection aims to be connected to all networks we support, to help the interconnection, and also to reduce the risk of eclipse attacks. Ideally, that means relaying b
...
💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443337020)
I tried it, but it turned out to be annoying because in the test we simulate being at the full outbound limit by artificially running with a lower `-maxconnections`. If we now also add an unrelated peer like ADDRFETCH , it would make the test fail because we don't have enough slots in `semOutbound`.
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443337020)
I tried it, but it turned out to be annoying because in the test we simulate being at the full outbound limit by artificially running with a lower `-maxconnections`. If we now also add an unrelated peer like ADDRFETCH , it would make the test fail because we don't have enough slots in `semOutbound`.
💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443337547)
done, thanks - I had missed this!
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443337547)
done, thanks - I had missed this!
💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443338184)
I think I'd prefer to not do it here, for my taste it's not related enough to the core of this PR.
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443338184)
I think I'd prefer to not do it here, for my taste it's not related enough to the core of this PR.
💬 maflcko commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443338498)
Sure, no need to change anything.
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443338498)
Sure, no need to change anything.
💬 maflcko commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1879217380)
I think the fuzz tests are also checking the exact hardware representation? So they should be failing as well, but I guess no one is running them through homebrew (or whatever the affected system is).
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1879217380)
I think the fuzz tests are also checking the exact hardware representation? So they should be failing as well, but I guess no one is running them through homebrew (or whatever the affected system is).
💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#issuecomment-1879217526)
Updated to address feedback, I would love to hear more opinions on the protection issue discussed in the threat following https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1429773492
(https://github.com/bitcoin/bitcoin/pull/28538#issuecomment-1879217526)
Updated to address feedback, I would love to hear more opinions on the protection issue discussed in the threat following https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1429773492
💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1879221008)
@maflcko I don't think they are affecteded, as the fuzz tests always start from a double (even if one obtained by decoding raw memory) and test roundtripping of that. The issue only seems to appear when starting from raw memory, then encoding + decoding, and comparing with the raw memory started with.
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1879221008)
@maflcko I don't think they are affecteded, as the fuzz tests always start from a double (even if one obtained by decoding raw memory) and test roundtripping of that. The issue only seems to appear when starting from raw memory, then encoding + decoding, and comparing with the raw memory started with.
💬 jonatack commented on pull request "Avoid returning references to mutex guarded members":
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1879232002)
> > Let me know if I should drop it and you prefer to keep it here
>
> It is ok. I don't have a preference whether it gets merged via #29040 of via this PR. Just that it makes it to `master` ;-) Thanks!
Need rebase, following merge of first commit cba94d151757a4e69b6eb684ae09bc6a4ea530d5 in #29040?
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1879232002)
> > Let me know if I should drop it and you prefer to keep it here
>
> It is ok. I don't have a preference whether it gets merged via #29040 of via this PR. Just that it makes it to `master` ;-) Thanks!
Need rebase, following merge of first commit cba94d151757a4e69b6eb684ae09bc6a4ea530d5 in #29040?
💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1879236472)
@maflcko I've added a commit to verify that... it adds a hardware-representation equivalence test again, but starting from a `double` like the fuzz tests do.
(https://github.com/bitcoin/bitcoin/pull/29192#issuecomment-1879236472)
@maflcko I've added a commit to verify that... it adds a hardware-representation equivalence test again, but starting from a `double` like the fuzz tests do.
💬 tcharding commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879236759)
> Might still be used here: https://github.com/rust-bitcoin/rust-bitcoinconsensus
>
> Ping @apoelstra @tcharding
`rust-bitcoinconsensus` can just do a "final" release using v27 and keep existing if folk want to use it, its trivial to maintain.
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879236759)
> Might still be used here: https://github.com/rust-bitcoin/rust-bitcoinconsensus
>
> Ping @apoelstra @tcharding
`rust-bitcoinconsensus` can just do a "final" release using v27 and keep existing if folk want to use it, its trivial to maintain.
👍 maflcko approved a pull request: "Weaken serfloat tests"
(https://github.com/bitcoin/bitcoin/pull/29192#pullrequestreview-1806821660)
lgtm
(https://github.com/bitcoin/bitcoin/pull/29192#pullrequestreview-1806821660)
lgtm
💬 maflcko commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443356658)
I think in C++20 a static_assert is evaluated even in a non-instantiated context, so could either make this global use the "template-hack" to not evaluate it if the context isn't instantiated either.
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443356658)
I think in C++20 a static_assert is evaluated even in a non-instantiated context, so could either make this global use the "template-hack" to not evaluate it if the context isn't instantiated either.
💬 sipa commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443361139)
Right.
I'll address this (and update the description of the PR which is now outdated with the last commit) when @fanquake reports this fixes #28941.
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443361139)
Right.
I'll address this (and update the description of the PR which is now outdated with the last commit) when @fanquake reports this fixes #28941.
💬 sr-gi commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-1879253826)
Concept ACK. Just a single consideration: wouldn't it also make sense to test this with `-logips=1`?
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-1879253826)
Concept ACK. Just a single consideration: wouldn't it also make sense to test this with `-logips=1`?
💬 ishaanam commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1443367173)
`self.def_wallet` is not equal to `self.wallet`, but they belong to the same node. I have implemented this suggestion, but given that the wallets are in the same node, will it make a difference?
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1443367173)
`self.def_wallet` is not equal to `self.wallet`, but they belong to the same node. I have implemented this suggestion, but given that the wallets are in the same node, will it make a difference?
💬 wizkid057 commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879268385)
> * Identifying extra data yet removing the witness discount rather than filtering it out entirely. It's not clear this would be effective alone, but is supported by Knots v25.1.
> * Adding a second `datacarriersize` with a broader scope like in [datacarriersize: Match more datacarrying #28408](https://github.com/bitcoin/bitcoin/pull/28408) ([suggested by glozow](https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1878972398)).
It seems like an acceptable compromise solution would b
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1879268385)
> * Identifying extra data yet removing the witness discount rather than filtering it out entirely. It's not clear this would be effective alone, but is supported by Knots v25.1.
> * Adding a second `datacarriersize` with a broader scope like in [datacarriersize: Match more datacarrying #28408](https://github.com/bitcoin/bitcoin/pull/28408) ([suggested by glozow](https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1878972398)).
It seems like an acceptable compromise solution would b
...
🤔 furszy reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1806935444)
Pre-review note: have you considered that `wallet_import_rescan.py` is a legacy wallet only test?
If it's placed here due to a lack of alternatives, I think this could be moved to a separate `wallet_mempool.py` file, where we could continue adding more cases related to the wallet-mempool interaction (or.. we could upgrade this file to run on a descriptors wallet).
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1806935444)
Pre-review note: have you considered that `wallet_import_rescan.py` is a legacy wallet only test?
If it's placed here due to a lack of alternatives, I think this could be moved to a separate `wallet_mempool.py` file, where we could continue adding more cases related to the wallet-mempool interaction (or.. we could upgrade this file to run on a descriptors wallet).
🤔 jonatack reviewed a pull request: "doc/reduce-traffic: update/clarify max outbound connection count"
(https://github.com/bitcoin/bitcoin/pull/29052#pullrequestreview-1806956443)
Post-merge review ACK with logging sanity check
```
2024-01-05T21:54:39.437118Z [Init] m_max_automatic_connections: 125
2024-01-05T21:54:39.437125Z [Init] m_max_outbound_full_relay: 8
2024-01-05T21:54:39.437133Z [Init] m_max_outbound_block_relay: 2
2024-01-05T21:54:39.437139Z [Init] m_max_automatic_outbound: 11
2024-01-05T21:54:39.437145Z [Init] m_max_inbound: 114
```
(https://github.com/bitcoin/bitcoin/pull/29052#pullrequestreview-1806956443)
Post-merge review ACK with logging sanity check
```
2024-01-05T21:54:39.437118Z [Init] m_max_automatic_connections: 125
2024-01-05T21:54:39.437125Z [Init] m_max_outbound_full_relay: 8
2024-01-05T21:54:39.437133Z [Init] m_max_outbound_block_relay: 2
2024-01-05T21:54:39.437139Z [Init] m_max_automatic_outbound: 11
2024-01-05T21:54:39.437145Z [Init] m_max_inbound: 114
```