🤔 glozow reviewed a pull request: "doc: refer to "Node relay options" in policy/README"
(https://github.com/bitcoin/bitcoin/pull/29235#pullrequestreview-1818040779)
lgtm ACK 0d627c4ca8684653ddef3bb4041ad1e129ed3d4d

✅ fanquake closed an issue: "`security-check.py` produces false positive result for stack smashing protection"
(https://github.com/bitcoin/bitcoin/issues/29084)

💬 fanquake commented on issue "`security-check.py` produces false positive result for stack smashing protection":
(https://github.com/bitcoin/bitcoin/issues/29084#issuecomment-1888967871)
The claim here is that if you change `-fstack-protector-all` to `-fno-stack-protector` in our compile flags, then `check_ELF_Canary` in `security-check.py` should fail during a Guix build.

However that wont happen, because the check looks for the presence of `__stack_chk_fail`, and that will still be in the binary, because libs in depends are compiled with the on-by-default stack-protector flags. This can easily be checked after Guix building with the diff above:
```bash
CXXFLAGS =
...

💬 Eunovo commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-1888973157)
ACK [7f0be89](https://github.com/bitcoin/bitcoin/pull/29156/commits/7f0be8969e721de69fc352f76db5787836803b76)

Adding an example for this to descriptors.md is really helpful but I think users who are unfamiliar with miniscript and descriptors might benefit from the addition of the fragments, `thresh` and `after` to the `Reference` section. Seeing these fragments used in the examples but not included the reference can introduce confusion.

🚀 glozow merged a pull request: "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0"
(https://github.com/bitcoin/bitcoin/pull/28885)

🚀 glozow merged a pull request: "doc: refer to "Node relay options" in policy/README"
(https://github.com/bitcoin/bitcoin/pull/29235)

💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1889089160)
Updated ffc1e9fd8cf53cfcf4d7926ca2963958eacf73d7 -> 3c311734d2fc6a4ca410f254ba3c8e923d58be70 ([`pr/iparams.3`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.3) -> [`pr/iparams.4`](https://github.com/ryanofsky/bitcoin/commits/pr/iparams.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/iparams.3..pr/iparams.4)) to fix unused variable in test https://cirrus-ci.com/task/4527517751574528?logs=ci#L2260

re: https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1888552974
...

🤔 shaavan reviewed a pull request: "init: handle empty settings file gracefully"
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1818189422)
Thanks for the update, @furszy
The new error message and the settings.json warnings are very precise in their details and I believe will be better in guiding the user in case of error, and refraining them from modifying settings without being cautious.

I just have one suggestion I want to add.

💬 shaavan commented on pull request "init: handle empty settings file gracefully":
(https://github.com/bitcoin/bitcoin/pull/29144#discussion_r1450404094)
Regarding commit 634be7d206b31d34b36de7e62bd216dbbc360195:

The commit message states:

> "...file unless they are certain about the potential consequences."

While this addition is valuable, the subsequent update only advises users against modifying the file while the node is running.
Considering this, would it be beneficial to think about revising the commit message to more effectively caution users before they adjust the settings? This would enhance clarity and help users make informed
...

💬 ryanofsky commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1450411706)
IMO, being able to filter log messages and only see messages from a particular component is a useful thing to support. Current bitcoin behavior of defining categories and not actually using them is weird, and wanting to bake this behavior into the logging API seems even weirder. The python logger is very flexible and of course it allows you to log per-component or globally. But if you did both these things in a single python project, it would be confusing and weird there too.

🤔 furszy reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1818093579)
Code reviewed, looking good. Left few comments, mostly testing time improvements. Nothing blocking.

As the scenario being covered does not require verifying p2p mempool relay, you could speed up the new `wallet_rescan_unconfirmed.py` test by ~2.5x (tested locally, ~3 secs versus ~8 secs) by using a single node instead of creating three of them. See commit f8273512 (feel free to pull it).

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450354940)
in https://github.com/bitcoin/bitcoin/commit/c39e82926a3740642f8f1bb72c6b10b67a6dc724:

nit: `disable_private_keys` is false by default. And, because this is a descriptors-only test (per `self.add_wallet_options`), the `descriptors` flag is also redundant.

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450376850)
In c39e82926a3:

Can simplify this by only importing the used address descriptor.
```suggestion
descriptors_to_import = [{"desc": w0.getaddressinfo(parent_address)['parent_desc'], "timestamp": 0, "label": "w0 import"}]
```

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450347036)
in c39e82926a:

tiny nit:
looks redundant to call `add_wallet_options(legacy=False)` and then call `skip_if_no_sqlite()`.
If the wallet is enabled and `sqlite` is not supported, then the only option is a legacy wallet, which is blocked by the `add_wallet_options` call.

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450417390)
Unless you specify a clean chain (which this test does not), the test will have an existing matured. Which means, you could modify this for

```suggestion
tester_wallet = MiniWallet(tester_node)
self.generate(tester_wallet, 1)
```

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450361941)
tiny nit: this could be `sync_mempools` instead.

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450366154)
In https://github.com/bitcoin/bitcoin/commit/c39e82926a3740642f8f1bb72c6b10b67a6dc724:

same as above, the `descriptors` flag isn't needed. Also, if you set `disable_private_keys=True`, there is no need to set `blank=True`.

💬 ryanofsky commented on pull request "init: handle empty settings file gracefully":
(https://github.com/bitcoin/bitcoin/pull/29144#discussion_r1450456098)
I think in general it is safe to edit the file, and main thing worth pointing out is when changes might be ignored or overwritten. It's possible adding more information could be useful, but I don't think there's anything clearly missing. The commit message also seems ok and wouldn't really be an appropriate place to caution users since they are unlikely to see it (in the file itself or in documentation would be better).

💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450457328)
I initially left the same comment about `disable_private_keys` but think it is helpful to contrast the difference with further down the test where we set `disable_private_keys=True`. Likewise, I think keeping `descriptors=True` is helpful self-documentation given the contrast with the test in `wallet_import_rescan.py`. No strong opinion on either, though.

👍 dergoegge approved a pull request: "log mempool loading progress"
(https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1818278096)
Code review ACK 6c3ca0cac38f3b9e307cd0522ba9e3b7a2bd05a5