👍 BrandonOdiwuor approved a pull request: "wallet: Reset chain notifications handler if AttachChain fails"
(https://github.com/bitcoin/bitcoin/pull/29243#pullrequestreview-1821161372)
Code Review ACK ea2551e55d260854a5cca8aa95034970d4adca1c
(https://github.com/bitcoin/bitcoin/pull/29243#pullrequestreview-1821161372)
Code Review ACK ea2551e55d260854a5cca8aa95034970d4adca1c
💬 S3RK commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1891638400)
Code review ACK c856a1093009c6098c7c9fed5cc3f03fd0c0cf5b
Nice improvements removing duplication between `CreateRecipient` and `AddOutputs`, also embedding SFFO values in `CRecipient` instead of passing as a separate parameter to `FundTransaction`.
I'd still prefer `InterpretSubtractFeeFromOutputInstructions` to work with one type of SFFO option - `vector<int>` and process others at the call site. SFFO option can be in three different forms and `vector<int>` is the only form that used multi
...
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1891638400)
Code review ACK c856a1093009c6098c7c9fed5cc3f03fd0c0cf5b
Nice improvements removing duplication between `CreateRecipient` and `AddOutputs`, also embedding SFFO values in `CRecipient` instead of passing as a separate parameter to `FundTransaction`.
I'd still prefer `InterpretSubtractFeeFromOutputInstructions` to work with one type of SFFO option - `vector<int>` and process others at the call site. SFFO option can be in three different forms and `vector<int>` is the only form that used multi
...
💬 fanquake commented on pull request "[POC] guix: produce a fully `-static-pie` bitcoind":
(https://github.com/bitcoin/bitcoin/pull/25573#issuecomment-1891768915)
> Looks like support in glibc will also be available soon: https://sourceware.org/pipermail/libc-alpha/2023-October/152132.html.
This has now landed: https://sourceware.org/git/?p=glibc.git;a=commit;h=e0590f41fe1e7a54169e8f8828efe62b5064139e.
(https://github.com/bitcoin/bitcoin/pull/25573#issuecomment-1891768915)
> Looks like support in glibc will also be available soon: https://sourceware.org/pipermail/libc-alpha/2023-October/152132.html.
This has now landed: https://sourceware.org/git/?p=glibc.git;a=commit;h=e0590f41fe1e7a54169e8f8828efe62b5064139e.
💬 fanquake commented on pull request "depends: remove dependency on Darwin libtool":
(https://github.com/bitcoin/bitcoin/pull/29232#discussion_r1452167939)
It's weird that miniupnpc would add a special build system option for setting a single, Windows-only define. It's not really clear what the benefits of taht are over just using `CPPFLAGS`, which should always work. The only rationale from the PR author for not doing that was [`I don't like it.`](https://github.com/miniupnp/miniupnp/pull/659#issuecomment-1585595562).
(https://github.com/bitcoin/bitcoin/pull/29232#discussion_r1452167939)
It's weird that miniupnpc would add a special build system option for setting a single, Windows-only define. It's not really clear what the benefits of taht are over just using `CPPFLAGS`, which should always work. The only rationale from the PR author for not doing that was [`I don't like it.`](https://github.com/miniupnp/miniupnp/pull/659#issuecomment-1585595562).
✅ fanquake closed an issue: "ci: failure in `wallet_assumeutxo.py --descriptors`"
(https://github.com/bitcoin/bitcoin/issues/29234)
(https://github.com/bitcoin/bitcoin/issues/29234)
🚀 fanquake merged a pull request: "wallet: Reset chain notifications handler if AttachChain fails"
(https://github.com/bitcoin/bitcoin/pull/29243)
(https://github.com/bitcoin/bitcoin/pull/29243)
💬 maflcko commented on pull request "test: Remove all-lint.py script":
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1891802590)
> > Seems confusing to have a test runner that calls another runner (`all-lint.py`), which calls a subset of the lint tests.
>
> However, it is quite convenient to run `./test/lint/all-lint.py` locally, no?
Why would it be convenient, when it is called "all-lint", but at the same time skips checks such as the fs check, the subtree check, and the doc check?
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1891802590)
> > Seems confusing to have a test runner that calls another runner (`all-lint.py`), which calls a subset of the lint tests.
>
> However, it is quite convenient to run `./test/lint/all-lint.py` locally, no?
Why would it be convenient, when it is called "all-lint", but at the same time skips checks such as the fs check, the subtree check, and the doc check?
🚀 fanquake merged a pull request: "doc: Add missing backtick in developer notes logging section"
(https://github.com/bitcoin/bitcoin/pull/29241)
(https://github.com/bitcoin/bitcoin/pull/29241)
💬 fanquake commented on pull request "rpc: Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#issuecomment-1891821253)
Concept ACK for 27.0.
(https://github.com/bitcoin/bitcoin/pull/29239#issuecomment-1891821253)
Concept ACK for 27.0.
💬 maflcko commented on pull request "rpc: Make v2transport default for addnode RPC when enabled":
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1452200703)
Missing test coverage seems unrelated to this pull? (This line and condition is not changed)
(https://github.com/bitcoin/bitcoin/pull/29239#discussion_r1452200703)
Missing test coverage seems unrelated to this pull? (This line and condition is not changed)
💬 maflcko commented on pull request "logging: Update to new logging API":
(https://github.com/bitcoin/bitcoin/pull/29231#issuecomment-1891833356)
> happy to open a separate pull for it too?
Yeah, I think it is better to change a line of code only once, instead of twice, unless there is a reason to change it twice.
(https://github.com/bitcoin/bitcoin/pull/29231#issuecomment-1891833356)
> happy to open a separate pull for it too?
Yeah, I think it is better to change a line of code only once, instead of twice, unless there is a reason to change it twice.
💬 torkelrogstad commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-1891915104)
I believe CI failures are unrelated. Is it possible to force CI to re-run?
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-1891915104)
I believe CI failures are unrelated. Is it possible to force CI to re-run?
💬 torkelrogstad commented on pull request "test: Remove all-lint.py script":
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1891924076)
> However, it is quite convenient to run `./test/lint/all-lint.py` locally, no?
Chiming in as a very inexperienced contributor that has been bashing against CI linter failures: I would say it's very nice to have an easy way to run all tests and checks locally before pushing. The correct thing to do here would be to _expand_ the linter script to include more of the checks, IMO.
On a slightly related note: it would also be nice to include either instructions or CLI flags that fix linter com
...
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1891924076)
> However, it is quite convenient to run `./test/lint/all-lint.py` locally, no?
Chiming in as a very inexperienced contributor that has been bashing against CI linter failures: I would say it's very nice to have an easy way to run all tests and checks locally before pushing. The correct thing to do here would be to _expand_ the linter script to include more of the checks, IMO.
On a slightly related note: it would also be nice to include either instructions or CLI flags that fix linter com
...
⚠️ fanquake opened an issue: "build: multiprocess compile failure on macOS"
(https://github.com/bitcoin/bitcoin/issues/29248)
master @ 28ccc7003a4c3f198a255d6f55f5748d4fd8fd1a
```bash
Apple clang version 15.0.0 (clang-1500.1.0.2.5)
make -C depends -j12 MULTIPROCESS=1 NO_QT=1 NO_WALLET=1 NO_NATPMP=1 NO_UPNP=1 NO_ZMQ=1
./autogen.sh
CONFIG_SITE=/bitcoin/depends/aarch64-apple-darwin23.2.0/share/config.site ./configure
make
```
```bash
In file included from ipc/capnp/echo.capnp.proxy-client.c++:3:
In file included from ./ipc/capnp/echo.capnp.proxy-types.h:6:
In file included from ./ipc/capnp/echo.capnp.prox
...
(https://github.com/bitcoin/bitcoin/issues/29248)
master @ 28ccc7003a4c3f198a255d6f55f5748d4fd8fd1a
```bash
Apple clang version 15.0.0 (clang-1500.1.0.2.5)
make -C depends -j12 MULTIPROCESS=1 NO_QT=1 NO_WALLET=1 NO_NATPMP=1 NO_UPNP=1 NO_ZMQ=1
./autogen.sh
CONFIG_SITE=/bitcoin/depends/aarch64-apple-darwin23.2.0/share/config.site ./configure
make
```
```bash
In file included from ipc/capnp/echo.capnp.proxy-client.c++:3:
In file included from ./ipc/capnp/echo.capnp.proxy-types.h:6:
In file included from ./ipc/capnp/echo.capnp.prox
...
💬 maflcko commented on pull request "test: Remove all-lint.py script":
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1891989227)
> The correct thing to do here would be to _expand_ the linter script to include more of the checks, IMO.
If you want to run all checks locally, you can refer to the documentation (https://github.com/bitcoin/bitcoin/blob/master/test/lint/README.md#running-locally). The options include running them in a docker container, or via the test_runner.
I am happy to close this pull, but `all-lint.py` never was a way to run "all" lint checks (the above mentioned were always excluded) and given that
...
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1891989227)
> The correct thing to do here would be to _expand_ the linter script to include more of the checks, IMO.
If you want to run all checks locally, you can refer to the documentation (https://github.com/bitcoin/bitcoin/blob/master/test/lint/README.md#running-locally). The options include running them in a docker container, or via the test_runner.
I am happy to close this pull, but `all-lint.py` never was a way to run "all" lint checks (the above mentioned were always excluded) and given that
...
💬 maflcko commented on pull request "test: Remove all-lint.py script":
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1891992233)
> I had issues with getting the whitespace linter to pass, and couldn't find any Makefile target, script or similar to automatically format my Python code.
I think you can use any automated formatter of your choice. For example `black` (for a file that was written fully fresh), or yapf-diff (see the docs in this repo), or any other automated formatter.
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1891992233)
> I had issues with getting the whitespace linter to pass, and couldn't find any Makefile target, script or similar to automatically format my Python code.
I think you can use any automated formatter of your choice. For example `black` (for a file that was written fully fresh), or yapf-diff (see the docs in this repo), or any other automated formatter.
💬 TheCharlatan commented on pull request "test: Remove all-lint.py script":
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1892022330)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29228#issuecomment-1892022330)
Concept ACK
💬 maflcko commented on pull request "wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs":
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-1892024499)
rebased
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-1892024499)
rebased
💬 furszy commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1452346405)
It is not immediately clear that additional services flags may be added, but the return value could still be false. And the same seems to happen for the `AddrInfo` timestamp. It can also be updated while the return value remains false.
(https://github.com/bitcoin/bitcoin/pull/29213#discussion_r1452346405)
It is not immediately clear that additional services flags may be added, but the return value could still be false. And the same seems to happen for the `AddrInfo` timestamp. It can also be updated while the return value remains false.
🚀 fanquake merged a pull request: "depends: Allow PATH with spaces in directory names."
(https://github.com/bitcoin/bitcoin/pull/29237)
(https://github.com/bitcoin/bitcoin/pull/29237)