💬 ajtowns commented on pull request "log mempool loading progress":
(https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1891521307)
> "I wonder why Bitcoin Core takes so long to shutdown sometimes?" repeatedly from users.
I don't think that's due to dumping the mempool, but rather spinning on locks elsewhere in the code. I've seen very slow shutdowns myself, but been unable to nail it down precisely.
(https://github.com/bitcoin/bitcoin/pull/29227#issuecomment-1891521307)
> "I wonder why Bitcoin Core takes so long to shutdown sometimes?" repeatedly from users.
I don't think that's due to dumping the mempool, but rather spinning on locks elsewhere in the code. I've seen very slow shutdowns myself, but been unable to nail it down precisely.
👍 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.