Bitcoin Core Github
42 subscribers
126K links
Download Telegram
🤔 mzumsande reviewed a pull request: "p2p: track and use all potential peers for orphan resolution"
(https://github.com/bitcoin/bitcoin/pull/31397#pullrequestreview-2554067385)
ACK 86d7135e36efd39781cf4c969011df99f0cbb69d

I reviewed the code and tested it a bit on mainnet with some extra logging: The large majority of orphans gets resolved, a few orphans get stuck for 20 minutes if all of our candidates send `NOTFOUND`s for the parent requests, presumably because the parent got removed from their mempools (this has been mentioned in https://github.com/bitcoin/bitcoin/pull/31397#issuecomment-2523122642 ).
💬 jonatack commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2594008381)
Concept ACK
💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#discussion_r1917448533)
Good idea, thanks. I've removed the implicit check.
📝 davidgumberg opened a pull request: "build: Make config warnings fatal if -DWERROR=ON"
(https://github.com/bitcoin/bitcoin/pull/31665)
While reviewing #31593, which upgrades the CentOS version in CI from Stream 9 to Stream 10, I discovered that the CentOS ci task had been attempting to configure a build with python `3.9` (the latest version in the Stream 9 core repo) below the minimum version of `3.10` which resulted in a warning being appended to the `configure_warnings` cmake/env variable. https://github.com/bitcoin/bitcoin/blob/712cab3a8f8ad76db959337ddc35cb4c34cac388/CMakeLists.txt#L546-L553

This warning has been emitted
...
💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2594120423)
Updated commit message since this commit now also removes an assertion as discussed in this PR review.
💬 davidgumberg commented on pull request "ci: Bump centos stream 10":
(https://github.com/bitcoin/bitcoin/pull/31593#issuecomment-2594133167)
Upstream tracking issue for branching and adding `dash` into EPEL 10 is here: https://bugzilla.redhat.com/show_bug.cgi?id=2335416
💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1917490318)
Thanks, done.
💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1917491100)
How to best make this compatible with podman and/or macos remains an open question, but I've switched to using `CI_IMAGE_PLATFORM` that gets passed to `docker build` and `docker run` as you suggested
💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#issuecomment-2594180974)
Rebased to address feedback, removed redundant arch specification from image names and switched to using `CI_IMAGE_PLATFORM="--platform os/arch"` that gets passed to `docker build` and `docker run` instead of using the `DOCKER_DEFAULT_PLATFORM` environment variable.
💬 theStack commented on pull request "test: p2p: fix sending of manual INVs in tx download test":
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2594215328)
> Good find! I think that `p2p_orphan_handling.py` has a similar [spot](https://github.com/bitcoin/bitcoin/blob/e7c479495509c068215b73f6df070af2d406ae15/test/functional/p2p_orphan_handling.py#L431).

Ah right, missed that one. If I interpret the `test_orphan_inherit_rejection` sub-test correctly, we want to send the INV with MSG_TX there to "announce again with potentially different witness", so the solution in this case to avoid ignoring the INV would be to add peer3 with wtxidrelay=False (ne
...
👍 tdb3 approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2554616902)
code review re ACK 712e3ffde5fa4fd26c0f217942b4a289cf37f361

Since last review, the test was updated to use spendable coinbase UTXOs.
📝 saikiran57 opened a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31667)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...
💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31667#issuecomment-2594758942)
I've fixed issue regarding https://github.com/bitcoin/bitcoin/issues/31263

now we can pass rescan option as a request params
saikiran57 closed a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31667)
📝 saikiran57 opened a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31668)
Fix related to issue: https://github.com/bitcoin/bitcoin/issues/31263
💬 hebasto commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2594865842)
Related to https://github.com/bitcoin/bitcoin/issues/31476.
💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2594870730)
Rebased and trying https://github.com/chaincodelabs/libmultiprocess/pull/127.
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918054673)
I don't know. @maflcko?
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918061002)
> This seems ok, but I think I don't understand why it is useful to test that coins are spendable.

It's basically checking against a bug I introduced in an earlier version. Initially I used taproot addresses for the coinbase which are unspendable due to the `unexpected-witness".

However it might be simpler if I drop it and just leave a separate commit as a comment in the PR, if someone needs it later.