💬 Sjors commented on pull request "Don't fix Python patch version":
(https://github.com/bitcoin/bitcoin/pull/33051#issuecomment-3117511064)
(added rationale to the first commit message)

🤔 janb84 reviewed a pull request: "refactor: GenTxid type safety followups"
(https://github.com/bitcoin/bitcoin/pull/33005#pullrequestreview-3055150696)
concept ACK 8e19a4375e1db6e59cdcb38e0cc3e1dab79e88ec

This PR is a followup of #32631 which addresses some of the NITS / comments that were placed in that PR. Additionally the PR simplifies `m_tx_inventory_to_send` a bit to make it `std::set<Wtxid>` based in stead of `std::set<GenTxid>` imho a good change.

- code reviewed ✅
- build and tested ✅

💬 maflcko commented on pull request "subprocess: always check result of close()":
(https://github.com/bitcoin/bitcoin/pull/33061#issuecomment-3117590501)
Also, it would be good to have evidence that the underlying bug is related to the close at all.

Even with code added to throw on a failed close, the error keeps happening for me:


diff:


```diff
diff --git a/ci/test/00_setup_env_i686_multiprocess.sh b/ci/test/00_setup_env_i686_multiprocess.sh
index e19a45d..4a7c3ea 100755
--- a/ci/test/00_setup_env_i686_multiprocess.sh
+++ b/ci/test/00_setup_env_i686_multiprocess.sh
@@ -16,6 +16,8 @@ export GOAL="install"
export TEST_RUNNER_EXT
...

💬 rkrux commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#discussion_r2230972628)
In 17bf90179097f0865500ff88b5f6d307127a8917 "tests: Check that the last hardened cache upgrade occurs"

I reluctantly find myself suggesting this because I believe it would be easier to have only one such implementation in the `test_framework.py` (similar to `skip_if_no_py_sqlite3`) for this instead of quickly duplicating in couple test files. Fine with any variant of this function that seems more suitable.

```python
def import_py_sqlite3():
try:
import sqlite3
return True
except
...

💬 yancyribbens commented on pull request "Slay BnB Mutants":
(https://github.com/bitcoin/bitcoin/pull/33060#issuecomment-3117671691)
Concept ACK. I recently found some of these by accident working on max-weight related projects.

💬 m3dwards commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2231013930)
Ideally CI will run without a patch in as many places as possible.

By checking the repository name it will run for: pushes/merges to main repo, PRs from forks and pushes to forks (where it would run on a github runner).

The `USE_CIRRUS_RUNNERS` enabled a fork to also run on Cirrus if they have bought some runners but would NOT work for a PR from a third fork to that fork. I think this is confusing and so I agree to remove this var. Forks would have to patch if they want to run on cirrus ru
...

💬 rkrux commented on pull request "wallet: Set descriptor cache upgraded flag for migrated wallets":
(https://github.com/bitcoin/bitcoin/pull/33031#issuecomment-3117737469)
I deleted my previous updated suggestion regarding importing `pysqlite3` because it is not considerably better than the current implementation.

💬 glozow commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2231065911)
I think most of the benefits of precomputing this would be realized in the loop a few lines down looking for a package_tx sibling. As is, I'm not sure how much faster precomputation is: n=2 in normal cases, and we don't need to continue computing parents once we've realized it's oversize.

💬 glozow commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2231070636)
Would you consider using DepGraph instead of a matrix of indices (it's basically the same thing but more compact)?

💬 darosior commented on pull request "net, validation: don't punish peers for consensus-invalid txs":
(https://github.com/bitcoin/bitcoin/pull/33050#discussion_r2231118764)
If only the non-standard error message is renamed, and "block-script-verify-flag-failed" is left as a followup, this should not meaningfully affect the size of the diff at all. Here is a diff which does this with your "mempool-script-verify-flag-failed" suggestion:
<details>
<summary>Expand patch</summary>

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 3acdc6f63a1..a73ebc56eeb 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2218,7 +2218,7 @@ bool CheckI
...

📝 maflcko opened a pull request: "util: Revert "common: Close non-std fds before exec in RunCommandJSON""
(https://github.com/bitcoin/bitcoin/pull/33063)
After a fork() in a multithreaded program, the child can safely
call only async-signal-safe functions (see [signal-safety(7)](https://www.man7.org/linux/man-pages/man7/signal-safety.7.html))
until such time as it calls execv.

The standard library (`std` namespace) is not async-signal-safe. Also, `throw`, isn't.

There was an alternative implementation using `readdir` (https://github.com/bitcoin/bitcoin/pull/32529), but that isn't async-signal-safe either, and that implementation was still
...

💬 Sjors commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3117880896)
I did some more testing with 587c63e06955ce5c5f2d64f1c1700ca8628572cf, noticed improvements:
- rescan uses the slow version (when `-blockfilterindex` is enabled), as expected
- if I add a label to a transaction, it stores it

New bug:
- when I send with change, using my wallet that only has an sp descriptor, the change output is dropped and added to fees

Some remaining issues:
- add silent payments to "output types" in the receive screen (see patch below)

Some style suggestions:
- I
...

💬 fanquake commented on pull request "util: Revert "common: Close non-std fds before exec in RunCommandJSON"":
(https://github.com/bitcoin/bitcoin/pull/33063#issuecomment-3117894742)
ACK - to stop the flow of CI failures.

cc @hebasto @laanwj.

💬 fanquake commented on pull request "common: Close non-std fds before exec in RunCommandJSON":
(https://github.com/bitcoin/bitcoin/pull/32343#issuecomment-3117894878)
Note that this is being reverted in #33063.

📝 fanquake opened a pull request: "test: fix RPC coverage check"
(https://github.com/bitcoin/bitcoin/pull/33064)
This is #27593 cleaned up / rebased, now that the legacy wallet has been dropped.

Currently a draft given:
```bash
ALL | ✓ Passed | 2752 s (accumulated)
Runtime: 194 s

Uncovered RPC commands:
- upgradewallet

Cleaning up coverage data
```

`upgradewallet` is likely to be removed in #32944.

Closes #27593.

💬 fanquake commented on pull request "test: fix RPC coverage check":
(https://github.com/bitcoin/bitcoin/pull/29387#issuecomment-3117940249)
Picked up in #33064.

💬 Sjors commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#discussion_r2231169665)
5be304dfa50bcfd3b0a92eff52bac3d8023ce523 nit: no need to set `next_index` during import

💬 Sjors commented on pull request "util: Revert "common: Close non-std fds before exec in RunCommandJSON"":
(https://github.com/bitcoin/bitcoin/pull/33063#discussion_r2231248253)
Maybe only change this and don't undo the already upstreamed change (yet)?

💬 Sjors commented on pull request "util: Revert "common: Close non-std fds before exec in RunCommandJSON"":
(https://github.com/bitcoin/bitcoin/pull/33063#issuecomment-3118093293)
> call only async-signal-safe functions

Is this related? https://github.com/arun11299/cpp-subprocess/issues/53

💬 Sjors commented on pull request "util: Revert "common: Close non-std fds before exec in RunCommandJSON"":
(https://github.com/bitcoin/bitcoin/pull/33063#discussion_r2231269913)
Oh never mind, we didn't upstream this ourselves, it was already there and we copied it here.