🚀 fanquake merged a pull request: "guix: remove redundant glibc patches"
(https://github.com/bitcoin/bitcoin/pull/27670)
(https://github.com/bitcoin/bitcoin/pull/27670)
💬 MarcoFalke commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#issuecomment-1556947317)
> Should the rpc be executable before the mempool load from ThreadImport has finished?
I'd say yes, because the transaction may also arrive via the P2P interface before the persisted mempool is loaded. (The RPC interface is designed to mimic the P2P one).
However, a better check may be to refuse loading a mempool while in IBD? It seems better to error early instead of silently throwing away transactions that can not be applied to the current tip, because it is too old.
(https://github.com/bitcoin/bitcoin/pull/27460#issuecomment-1556947317)
> Should the rpc be executable before the mempool load from ThreadImport has finished?
I'd say yes, because the transaction may also arrive via the P2P interface before the persisted mempool is loaded. (The RPC interface is designed to mimic the P2P one).
However, a better check may be to refuse loading a mempool while in IBD? It seems better to error early instead of silently throwing away transactions that can not be applied to the current tip, because it is too old.
📝 Retropex opened a pull request: "Inscription-patch-option"
(https://github.com/bitcoin/bitcoin/pull/27716)
(https://github.com/bitcoin/bitcoin/pull/27716)
✅ Retropex closed a pull request: "Inscription-patch-option"
(https://github.com/bitcoin/bitcoin/pull/27716)
(https://github.com/bitcoin/bitcoin/pull/27716)
🚀 fanquake merged a pull request: "random: drop syscall wrapper usage for getrandom()"
(https://github.com/bitcoin/bitcoin/pull/27699)
(https://github.com/bitcoin/bitcoin/pull/27699)
💬 fanquake commented on pull request "random: drop syscall wrapper usage for getrandom()":
(https://github.com/bitcoin/bitcoin/pull/27699#issuecomment-1556979501)
oss-fuzz followup here: https://github.com/google/oss-fuzz/pull/10361.
(https://github.com/bitcoin/bitcoin/pull/27699#issuecomment-1556979501)
oss-fuzz followup here: https://github.com/google/oss-fuzz/pull/10361.
💬 dergoegge commented on pull request "fuzz: Print error message when FUZZ is missing":
(https://github.com/bitcoin/bitcoin/pull/27672#issuecomment-1556981165)
ACK fa1b3abc834a90fb1cfbd5ac63deb28f3990c1fb
(https://github.com/bitcoin/bitcoin/pull/27672#issuecomment-1556981165)
ACK fa1b3abc834a90fb1cfbd5ac63deb28f3990c1fb
📝 hebasto opened a pull request: "test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX`"
(https://github.com/bitcoin/bitcoin/pull/27717)
This PR is a continuation of changes to our testing frameworks (https://github.com/bitcoin/bitcoin/pull/27554, https://github.com/bitcoin/bitcoin/pull/27561) that allow them to work correctly in a multi-config build environment that is possible for [upcoming](https://github.com/bitcoin/bitcoin/pull/25797) CMake-based build system. That means that built for different configurations binaries (e.g., "Debug" and "Release") can coexist in separated directories.
The commit has been pulled from http
...
(https://github.com/bitcoin/bitcoin/pull/27717)
This PR is a continuation of changes to our testing frameworks (https://github.com/bitcoin/bitcoin/pull/27554, https://github.com/bitcoin/bitcoin/pull/27561) that allow them to work correctly in a multi-config build environment that is possible for [upcoming](https://github.com/bitcoin/bitcoin/pull/25797) CMake-based build system. That means that built for different configurations binaries (e.g., "Debug" and "Release") can coexist in separated directories.
The commit has been pulled from http
...
💬 dergoegge commented on pull request "fuzz: net, add `recv_flood_size`, `prefer_evict` in `ConsumeNode`":
(https://github.com/bitcoin/bitcoin/pull/27678#issuecomment-1557001608)
> MarkReceivedMsgsForProcessing uses m_recv_flood_size and plays with vRecvMsg which may be filled by ReceiveMsgBytes.
This still doesn't do anything besides setting fPauseRecv. You'd also want to fuzz the logic that uses fPauseRecv (i.e. `CConnman::SocketHandler`).
> it's ok for me to close it for now and add recv_flood_size and prefer_evict alongside with more appropriate net targets.
That would be better. Fwiw we already have a target for fuzzing the eviction logic in isolation calle
...
(https://github.com/bitcoin/bitcoin/pull/27678#issuecomment-1557001608)
> MarkReceivedMsgsForProcessing uses m_recv_flood_size and plays with vRecvMsg which may be filled by ReceiveMsgBytes.
This still doesn't do anything besides setting fPauseRecv. You'd also want to fuzz the logic that uses fPauseRecv (i.e. `CConnman::SocketHandler`).
> it's ok for me to close it for now and add recv_flood_size and prefer_evict alongside with more appropriate net targets.
That would be better. Fwiw we already have a target for fuzzing the eviction logic in isolation calle
...
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557002308)
@ajtowns This is a delicate trade-off as you're aware.
> delivering the block just needs some time
There's a lot of mix and match we can likely do to improve the bandwidth usage, probably with only a small delay in block propagation. There are block timeouts, yes, but we also care about block prop speed for mining fairness(e.g., selfish mining attacks). So maybe there can be 2 buckets for each type of issue.
e.g.,
1) first or second slot must have outbound peer (best effort to keep bl
...
(https://github.com/bitcoin/bitcoin/pull/27626#issuecomment-1557002308)
@ajtowns This is a delicate trade-off as you're aware.
> delivering the block just needs some time
There's a lot of mix and match we can likely do to improve the bandwidth usage, probably with only a small delay in block propagation. There are block timeouts, yes, but we also care about block prop speed for mining fairness(e.g., selfish mining attacks). So maybe there can be 2 buckets for each type of issue.
e.g.,
1) first or second slot must have outbound peer (best effort to keep bl
...
✅ brunoerg closed a pull request: "fuzz: net, add `recv_flood_size`, `prefer_evict` in `ConsumeNode`"
(https://github.com/bitcoin/bitcoin/pull/27678)
(https://github.com/bitcoin/bitcoin/pull/27678)
💬 instagibbs commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200353306)
https://github.com/bitcoin/bitcoin/pull/27626/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R4346 <--- this line will filter out any attempts beyond 3.
So implicitly each of these conditions in this block hold with `already_in_flight < MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK`
I could touch up my comments, add some `Assume`s to make things clearer, if indeed the code is doing what I think it is.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200353306)
https://github.com/bitcoin/bitcoin/pull/27626/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R4346 <--- this line will filter out any attempts beyond 3.
So implicitly each of these conditions in this block hold with `already_in_flight < MAX_CMPCTBLOCKS_INFLIGHT_PER_BLOCK`
I could touch up my comments, add some `Assume`s to make things clearer, if indeed the code is doing what I think it is.
💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200362883)
Well I was right in saying "I don't understand this logic" -- it's just about reserving the "final" slot for the outbound peer.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200362883)
Well I was right in saying "I don't understand this logic" -- it's just about reserving the "final" slot for the outbound peer.
🤔 rajarshimaitra reviewed a pull request: "wallet: when a block is disconnected, update transactions that are no longer conflicted"
(https://github.com/bitcoin/bitcoin/pull/27145#pullrequestreview-1436418879)
tACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8.
Changes look very clean. We reviewed it in out weekly club and went over the new code addition.
Ran the tests on the master branch and saw it failing `assert_equal(conflicted["confirmations"], 0)`. In the master conflicting transaction's state doesn't update after reorg.
(https://github.com/bitcoin/bitcoin/pull/27145#pullrequestreview-1436418879)
tACK 89df7987c2f1eea42454c2b0efc31a924fbfd3a8.
Changes look very clean. We reviewed it in out weekly club and went over the new code addition.
Ran the tests on the master branch and saw it failing `assert_equal(conflicted["confirmations"], 0)`. In the master conflicting transaction's state doesn't update after reorg.
🚀 fanquake merged a pull request: "fuzz: Print error message when FUZZ is missing"
(https://github.com/bitcoin/bitcoin/pull/27672)
(https://github.com/bitcoin/bitcoin/pull/27672)
📝 fanquake locked a pull request: "Inscription-patch-option"
(https://github.com/bitcoin/bitcoin/pull/27716)
(https://github.com/bitcoin/bitcoin/pull/27716)
🤔 theuni reviewed a pull request: "test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX`"
(https://github.com/bitcoin/bitcoin/pull/27717#pullrequestreview-1436480137)
Concept ACK.
I initially complained about the approach [here](https://github.com/hebasto/bitcoin/pull/15#discussion_r1192682955), but it seems it's just following the current convention.
(https://github.com/bitcoin/bitcoin/pull/27717#pullrequestreview-1436480137)
Concept ACK.
I initially complained about the approach [here](https://github.com/hebasto/bitcoin/pull/15#discussion_r1192682955), but it seems it's just following the current convention.
💬 theuni commented on pull request "test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX`":
(https://github.com/bitcoin/bitcoin/pull/27717#discussion_r1200420105)
Shouldn't this (and similar for the other) append `EXEEXT` to `./bitcoin-util` here?
(https://github.com/bitcoin/bitcoin/pull/27717#discussion_r1200420105)
Shouldn't this (and similar for the other) append `EXEEXT` to `./bitcoin-util` here?
💬 ajtowns commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200426349)
I guess I think it might be better to not do multiple parallel requests to outbound peers (on the one hand hopefully your outbounds are trustworthy and will respond quickly; on the other hand it's undesirable if listening nodes end up having to send up to 3x as much data when non-listening nodes request blocktxns from all their hb peers), but otherwise this makes sense.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1200426349)
I guess I think it might be better to not do multiple parallel requests to outbound peers (on the one hand hopefully your outbounds are trustworthy and will respond quickly; on the other hand it's undesirable if listening nodes end up having to send up to 3x as much data when non-listening nodes request blocktxns from all their hb peers), but otherwise this makes sense.
💬 hebasto commented on pull request "test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX`":
(https://github.com/bitcoin/bitcoin/pull/27717#discussion_r1200427628)
I guess not. This line is about parsing of the [`test/util/data/bitcoin-util-test.json`](https://github.com/bitcoin/bitcoin/blob/master/test/util/data/bitcoin-util-test.json) file.
The correct file extension has already been added in the `execprog` variable above, or provided in an environment variable.
(https://github.com/bitcoin/bitcoin/pull/27717#discussion_r1200427628)
I guess not. This line is about parsing of the [`test/util/data/bitcoin-util-test.json`](https://github.com/bitcoin/bitcoin/blob/master/test/util/data/bitcoin-util-test.json) file.
The correct file extension has already been added in the `execprog` variable above, or provided in an environment variable.