📝 glozow opened a pull request: "[NO MERGE] BIP331 Ancestor Package Relay"
(https://github.com/bitcoin/bitcoin/pull/27742)
**WORK IN PROGRESS.** Please do not run it for any use other than testing.
This PR is not meant for merge. This branch exists to help reviewers see what package relay would look like all together. I will open separate PRs for these components and expect to add more tests, docs, and polish along the way. This PR contains all of the functionality built in a linear manner. However, since there are pieces in various areas of the codebase and they can make progress in parallel, commits don't nec
...
(https://github.com/bitcoin/bitcoin/pull/27742)
**WORK IN PROGRESS.** Please do not run it for any use other than testing.
This PR is not meant for merge. This branch exists to help reviewers see what package relay would look like all together. I will open separate PRs for these components and expect to add more tests, docs, and polish along the way. This PR contains all of the functionality built in a linear manner. However, since there are pieces in various areas of the codebase and they can make progress in parallel, commits don't nec
...
💬 stickies-v commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204062504)
nit: I think you also need to `#include <util/translation.h>` for `bilingual_str`
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204062504)
nit: I think you also need to `#include <util/translation.h>` for `bilingual_str`
🤔 stickies-v reviewed a pull request: "kernel: Remove util/system from kernel library, interface_ui from validation."
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1441753577)
Code Review ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e
I'm still familiarising myself with the libbitcoinkernel project so for now I've mostly focused on the code being correct, readable, ... and not so much on architecture (which is arguably the most important thing here) etc. All the changes made sense, and are very well structured and readable.
I've left a few nits (+[here](https://github.com/TheCharlatan/bitcoin/commit/7d3b35004b039f2bd606bb46a540de7babdbc41e#commitcomment-114818127)
...
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1441753577)
Code Review ACK 7d3b35004b039f2bd606bb46a540de7babdbc41e
I'm still familiarising myself with the libbitcoinkernel project so for now I've mostly focused on the code being correct, readable, ... and not so much on architecture (which is arguably the most important thing here) etc. All the changes made sense, and are very well structured and readable.
I've left a few nits (+[here](https://github.com/TheCharlatan/bitcoin/commit/7d3b35004b039f2bd606bb46a540de7babdbc41e#commitcomment-114818127)
...
💬 stickies-v commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204065433)
nit: Do we need this?
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204065433)
nit: Do we need this?
💬 stickies-v commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204082373)
nit: could use `warn()` to avoid having an identical fn and param name (also more intuitive for a fn name imo, but that's personal)
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204082373)
nit: could use `warn()` to avoid having an identical fn and param name (also more intuitive for a fn name imo, but that's personal)
💬 stickies-v commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204148265)
nit: I think this include is no longer necessary
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204148265)
nit: I think this include is no longer necessary
💬 stickies-v commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204278405)
nit: I think adding line number is unmaintainable, would suggest leaving it out (+ a few lines down). Even the filename is probably not even necessary, but less of a problem.
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1204278405)
nit: I think adding line number is unmaintainable, would suggest leaving it out (+ a few lines down). Even the filename is probably not even necessary, but less of a problem.
💬 achow101 commented on pull request "Deniability - a tool to automatically improve coin ownership privacy":
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561460052)
It seems like a feature like this would be better to integrate into the wallet directly rather than something that is done through and managed by the GUI.
(https://github.com/bitcoin-core/gui/pull/733#issuecomment-1561460052)
It seems like a feature like this would be better to integrate into the wallet directly rather than something that is done through and managed by the GUI.
🚀 achow101 merged a pull request: "rpc: Fix invalid bech32 address handling"
(https://github.com/bitcoin/bitcoin/pull/27727)
(https://github.com/bitcoin/bitcoin/pull/27727)
💬 achow101 commented on pull request "25.0 Final Changes":
(https://github.com/bitcoin/bitcoin/pull/27686#issuecomment-1561508601)
ACK ac8d72668c7bbd9f8771442fe25fd37f08c3f5ae
(https://github.com/bitcoin/bitcoin/pull/27686#issuecomment-1561508601)
ACK ac8d72668c7bbd9f8771442fe25fd37f08c3f5ae
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204429122)
cce96182ba2457335868c65dc16b081c3dee32ee: the goal of the above loop seems to be to find one entry: the `vBlocksInFlight` entry for this particular block and `from_peer`. A followup could move that into its own function, and/or have the subsequent operations on the entry live _outside_ the loop.
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204429122)
cce96182ba2457335868c65dc16b081c3dee32ee: the goal of the above loop seems to be to find one entry: the `vBlocksInFlight` entry for this particular block and `from_peer`. A followup could move that into its own function, and/or have the subsequent operations on the entry live _outside_ the loop.
💬 Sjors commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204472811)
It ended up in 03423f8bd12b95a06a4a9d8377e781625dd38aae
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1204472811)
It ended up in 03423f8bd12b95a06a4a9d8377e781625dd38aae
💬 achow101 commented on pull request "test: Disable legacy wallet for mempool_packages.py":
(https://github.com/bitcoin/bitcoin/pull/27735#issuecomment-1561578795)
Is there a reason that this test needs the wallet in the first place?
(https://github.com/bitcoin/bitcoin/pull/27735#issuecomment-1561578795)
Is there a reason that this test needs the wallet in the first place?
💬 fanquake commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#discussion_r1204513106)
> -DLLVM_INCLUDE_BENCHMARKS=OFF -DLLVM_INCLUDE_TESTS=OFF to reduce CPU?
These two control the generation of build targets, however nothing should be getting compiled, as `LLVM_BUILD_TESTS` and `LLVM_BUILD_BENCHMARKS` [both default to `OFF`](https://llvm.org/docs/CMake.html). However I've also added some additional `-DLLVM_INCLUDE_*` options that should reduce compilation.
> -DLLVM_USE_LINKER=lld and/or -DLLVM_PARALLEL_LINK_JOBS=1 to reduce change of OOM?
I could follow up with a change
...
(https://github.com/bitcoin/bitcoin/pull/27737#discussion_r1204513106)
> -DLLVM_INCLUDE_BENCHMARKS=OFF -DLLVM_INCLUDE_TESTS=OFF to reduce CPU?
These two control the generation of build targets, however nothing should be getting compiled, as `LLVM_BUILD_TESTS` and `LLVM_BUILD_BENCHMARKS` [both default to `OFF`](https://llvm.org/docs/CMake.html). However I've also added some additional `-DLLVM_INCLUDE_*` options that should reduce compilation.
> -DLLVM_USE_LINKER=lld and/or -DLLVM_PARALLEL_LINK_JOBS=1 to reduce change of OOM?
I could follow up with a change
...
💬 achow101 commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1204518144)
AFAIK there aren't any other external signer software yet.
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1204518144)
AFAIK there aren't any other external signer software yet.
💬 achow101 commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1561612282)
ACK e3d02f8ef8cfe1d3c502ede863d14f82212a0ce6
(https://github.com/bitcoin/bitcoin/pull/24313#issuecomment-1561612282)
ACK e3d02f8ef8cfe1d3c502ede863d14f82212a0ce6
👋 brunoerg's pull request is ready for review: "net, refactor: net_processing, add `ProcessCompactBlockTxns`"
(https://github.com/bitcoin/bitcoin/pull/26969)
(https://github.com/bitcoin/bitcoin/pull/26969)
💬 instagibbs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561640287)
@ajtowns
(https://github.com/bitcoin/bitcoin/pull/26969#issuecomment-1561640287)
@ajtowns
💬 pinheadmz commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1204534315)
updated test and also included warning in release note
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1204534315)
updated test and also included warning in release note
🤔 Sjors reviewed a pull request: "Parallel compact block downloads, take 3"
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1442325175)
Post merge utACK d7f359b35e8b1e9acc4d397de262cd9ba9bbcb83 modulo question about missing `return`. Other comments can wait for a followup.
Conceptually this PR still relies on our node immediately reacting to an incoming compact block message. This forces us to be a bit impatient and potentially use more bandwidth than needed.
In a followup we could instead track which peers have announced a block and then request block transactions from other peers if the first peer takes too long. This co
...
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1442325175)
Post merge utACK d7f359b35e8b1e9acc4d397de262cd9ba9bbcb83 modulo question about missing `return`. Other comments can wait for a followup.
Conceptually this PR still relies on our node immediately reacting to an incoming compact block message. This forces us to be a bit impatient and potentially use more bandwidth than needed.
In a followup we could instead track which peers have announced a block and then request block transactions from other peers if the first peer takes too long. This co
...