💬 Davidson-Souza commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879130051)
Thank you, @1thales, for the ping.
I'm building a lightweight Bitcoin full node called [Floresta](https://github.com/Davidson-Souza/Floresta), and I'm using `libbitcoinconsensus` through `rust-bitcoinconsensus`. I would rather not reimplement the entire script interpreter logic, given the complexity of keeping this code consensus-compatible with core. I did reimplement some consensus logic, but tx validation and script interpreter are the hardest ones to. `libbitcoinconsensus` has some issues
...
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879130051)
Thank you, @1thales, for the ping.
I'm building a lightweight Bitcoin full node called [Floresta](https://github.com/Davidson-Souza/Floresta), and I'm using `libbitcoinconsensus` through `rust-bitcoinconsensus`. I would rather not reimplement the entire script interpreter logic, given the complexity of keeping this code consensus-compatible with core. I did reimplement some consensus logic, but tx validation and script interpreter are the hardest ones to. `libbitcoinconsensus` has some issues
...
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1443276867)
Probably yeah, tests should catch this case regardless. Will add one after deciding what way to go with heuristic vs diagram check
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1443276867)
Probably yeah, tests should catch this case regardless. Will add one after deciding what way to go with heuristic vs diagram check
💬 jamesob commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879168582)
Concept ACK
> If libbitcoinkernel exposes stateless block/tx validation, that is, I give the transaction[s], the inputs and some context like block hash and MTP, and it spits out a "valid" or "not valid because X"; I would start using it as replacement without problems.
Sounds pretty achievable in the short-term, and something that should be exposed for cross-implementation testing anyway?
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1879168582)
Concept ACK
> If libbitcoinkernel exposes stateless block/tx validation, that is, I give the transaction[s], the inputs and some context like block hash and MTP, and it spits out a "valid" or "not valid because X"; I would start using it as replacement without problems.
Sounds pretty achievable in the short-term, and something that should be exposed for cross-implementation testing anyway?
💬 maflcko commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1879175078)
> Why was it rebased? Just seems to make it harder to review the changes...
`git range-diff bitcoin-core/master a b` does not care about rebases, see the docs in this repo.
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1879175078)
> Why was it rebased? Just seems to make it harder to review the changes...
`git range-diff bitcoin-core/master a b` does not care about rebases, see the docs in this repo.
💬 brunoerg commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1879176485)
Pushed to fix CI.
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1879176485)
Pushed to fix CI.
💬 sr-gi 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_r1443312604)
> not super clear why we won't do this if full_outbound_delta > 0?
> Which raises the question whether there should be any interaction?
I think this would create a weird interaction. The current approach of this PR is trying to make the 8 full-outbound connections relay transactions, given we already have blocks-only connections, so we should not waste full-outbound slots for this purpose. However, our full-outbound connection protection criteria is based on whether they have provided vali
...
(https://github.com/bitcoin/bitcoin/pull/28538#discussion_r1443312604)
> not super clear why we won't do this if full_outbound_delta > 0?
> Which raises the question whether there should be any interaction?
I think this would create a weird interaction. The current approach of this PR is trying to make the 8 full-outbound connections relay transactions, given we already have blocks-only connections, so we should not waste full-outbound slots for this purpose. However, our full-outbound connection protection criteria is based on whether they have provided vali
...
💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1879183816)
>`git range-diff bitcoin-core/master a b` does not care about rebases, see the docs in this repo.
It's also painful to review. If there's no benefit to a rebase, better to just avoid it.
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1879183816)
>`git range-diff bitcoin-core/master a b` does not care about rebases, see the docs in this repo.
It's also painful to review. If there's no benefit to a rebase, better to just avoid it.
📝 sipa opened a pull request: "Weaken serfloat tests"
(https://github.com/bitcoin/bitcoin/pull/29192)
Closes #28941.
Our current tests for serfloat verify to distinct things:
* Whether they roundtrip `double`->`uint64_t`->`double` (excluding NaN values) on all systems.
* Whether on systems with a typical floating point unit that encoding matches the hardware representation, as before v22.0, we would dump the hardware representation directly to disk and we wanted to retain compatibility with that.
#28941 seems to show that the second property doesn't always hold, but just for "subnormal"
...
(https://github.com/bitcoin/bitcoin/pull/29192)
Closes #28941.
Our current tests for serfloat verify to distinct things:
* Whether they roundtrip `double`->`uint64_t`->`double` (excluding NaN values) on all systems.
* Whether on systems with a typical floating point unit that encoding matches the hardware representation, as before v22.0, we would dump the hardware representation directly to disk and we wanted to retain compatibility with that.
#28941 seems to show that the second property doesn't always hold, but just for "subnormal"
...
👍 jamesob approved a pull request: "fuzz: a target for the block index database"
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-1806749752)
ACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b ([`jamesob/ackr/28209.1.darosior.fuzz_a_target_for_the_bl`](https://github.com/jamesob/bitcoin/tree/ackr/28209.1.darosior.fuzz_a_target_for_the_bl))
Ran the test locally and read over the change. This is a good basic test to have in place; seems like a low-risk merge.
<details><summary>Show signature data</summary>
<p>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b ([`jamesob/ackr/28209.
...
(https://github.com/bitcoin/bitcoin/pull/28209#pullrequestreview-1806749752)
ACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b ([`jamesob/ackr/28209.1.darosior.fuzz_a_target_for_the_bl`](https://github.com/jamesob/bitcoin/tree/ackr/28209.1.darosior.fuzz_a_target_for_the_bl))
Ran the test locally and read over the change. This is a good basic test to have in place; seems like a low-risk merge.
<details><summary>Show signature data</summary>
<p>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK 8083aa21a699cb240a11741be8ba5cfeeb58ee8b ([`jamesob/ackr/28209.
...
💬 jamesob commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1443317367)
Good suggestion, but could be done in a follow-up I guess.
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1443317367)
Good suggestion, but could be done in a follow-up I guess.
💬 sipa commented on issue "brew: serfloat_tests tests fail on Linux":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1879199378)
Can someone try if #29192 addresses this problem?
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1879199378)
Can someone try if #29192 addresses this problem?
💬 maflcko commented on pull request "Weaken serfloat tests":
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443325481)
side-note: This should always be true, according to the static_assert:
```
src/compat/assumptions.h:static_assert(std::numeric_limits<float>::is_iec559, "IEEE 754 float assumed");
src/compat/assumptions.h:static_assert(std::numeric_limits<double>::is_iec559, "IEEE 754 double assumed");
(https://github.com/bitcoin/bitcoin/pull/29192#discussion_r1443325481)
side-note: This should always be true, according to the static_assert:
```
src/compat/assumptions.h:static_assert(std::numeric_limits<float>::is_iec559, "IEEE 754 float assumed");
src/compat/assumptions.h:static_assert(std::numeric_limits<double>::is_iec559, "IEEE 754 double assumed");
💬 fanquake commented on issue "brew: serfloat_tests tests fail on Linux":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1879205868)
I'll push that branch into my downstream PR.
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1879205868)
I'll push that branch into my downstream PR.
💬 andrewtoth commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1443331819)
Rebased
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1443331819)
Rebased
💬 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.