💬 instagibbs commented on pull request "Bugfix: net_processing: Restore "Already requested" error for FetchBlock":
(https://github.com/bitcoin/bitcoin/pull/28055#discussion_r1258724942)
example test covering the behavior I'm mentioning

💬 theuni commented on issue "Use semantic analysis in lint-logs.py":
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1629581578)
See [here](https://github.com/theuni/bitcoin-core-clang-plugin/commit/05fb23f21df32df4445951123d281172b12ee61b) for an example of this implemented as a `clang` plugin rather than `clang-tidy`. As noted above, it's pretty rough because LLVM does not yet expose several useful helpers.

Advantages:
- We can invent our own attributes, which is way nicer for future-proofing. See [here](https://github.com/theuni/bitcoin-tidy-experiments/pull/1) for an example of the kind of synchronization problems
...

💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629627284)
Hey there, is this PR ready to merge if there aren't any errors?

💬 brunoerg commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#discussion_r1258853476)
```suggestion
if (!SanityCheckASMap(asmap, 128)) return FuzzResult::UNINTERESTING;
```

💬 sipa commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629690355)
People will need to review it.

👋 achow101's pull request is ready for review: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286)

💬 brunoerg commented on pull request "Add support for "partial" fuzzers that indicate usefulness":
(https://github.com/bitcoin/bitcoin/pull/27552#issuecomment-1629809606)
I did a quick test. I suppose that with this new approach, `miniscript_script` corpus will contain only valid miniscripts, this sounds good. So, I first ran: `FUZZ=miniscript_script src/test/fuzz/fuzz new_corpus -runs=1000000` and then I ran `rpc` (`decodescript`) with the corpus from `miniscript_script` - `FUZZ=rpc LIMIT_TO_RPC_COMMAND=decodescript src/test/fuzz/fuzz new_corpus -runs=1`. It worked as expected!

Also "fuzzed" `decodescript` RPC using the following script:
```py
#!/usr/bin/en
...

📝 Mdashad071192 opened a pull request: "Create generator-generic-ossf-slsa3-publish.yml"
(https://github.com/bitcoin-core/gui/pull/746)

<!--
*** 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 impr
...

💬 ryanofsky commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1259011316)
> Also, it would be good to explain why in the following commits it is fine to then ignore the exceptions and no longer catch them.

I probably just missed something, but there shouldn't be places in the following commits where exceptions are no longer caught. This should just be a refactoring.

I agree though that throwing undocumented exceptions is not good. And probably getting rid of exceptions and using nodiscard return values would be better than documenting them. The reason these are
...

📝 ryanofsky converted_to_draft a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051)
This change just removes some code and gets rid of an unnecessary layer of indirection after #27861

🤔 ryanofsky reviewed a pull request: "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly"
(https://github.com/bitcoin/bitcoin/pull/28051#pullrequestreview-1520779536)
Making this into a draft because I want to do more work to avoid the uncaught exceptions that Marco mentioned https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1255496707, and address initialization order issues TheCharlatan mentioned https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1257286292 and https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1257291194

Updated d9453bddd6ce5446552e844f4d5b65368d831656 -> 24169b10d8df29684eaf6fd261370ccc0a940f30 ([`pr/noshut.2`](ht
...

💬 ryanofsky commented on pull request "Get rid of shutdown.cpp/shutdown.h, use SignalInterrupt directly":
(https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1257346188)
re: https://github.com/bitcoin/bitcoin/pull/28051#discussion_r1257259643

> Why not use `EnsureAnyNodeContext` here as done in many other places?

Thanks switched to that. I was trying to find the function and couldn't find it. I just assumed it had been lost in some refactoring.

💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258979349)
re: https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258032967

> nit

Thanks, fixed

💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258979021)
re: https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258586027

> nit: would `SetNotificationArgs` be a better name to clarify the side effects this function has?

It seems like if you just saw a `ReadNotificationArgs(args, *node.notifications)` call with no context you would assume that it is reading args from the "args" variable into the "notifications" variable. Is there another way it could be interpreted?

`node::ReadNotificationArgs` function name is meant to be consistent
...

🤔 ryanofsky reviewed a pull request: "kernel: Remove StartShutdown calls from validation code"
(https://github.com/bitcoin/bitcoin/pull/28048#pullrequestreview-1523139721)
Updated 09938a41d904c05b4676b064da9baa85e53e3e6f -> 3e6b6877645f978b4999efdfb9e616f4dbf9dd19 ([`pr/stopafter.3`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.3) -> [`pr/stopafter.4`](https://github.com/ryanofsky/bitcoin/commits/pr/stopafter.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/stopafter.3..pr/stopafter.4)) just fixing a spelling mistake that was pointed out

💬 ryanofsky commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1259001998)
re: https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1258600239

> I don't see in which scenario `blockTip()` would return `kernel::Interrupted` here, but the docs seem to indicate that it could. I'd suggest to either update the docs to reflect that we never expect this to interrupt, or add a logging statement similar to what's done in `ThreadImport`?

I'm happy to update the comment, but it would help to have a specific suggestion, because to me this is only saying that a kernel::I
...

🤔 ismaelsadeeq reviewed a pull request: "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/28027#pullrequestreview-1523177321)
Running this test failed on my device not sure if it's from this PR though.

```
2023-07-11T00:04:00.468000Z TestFramework (INFO): PRNG seed is: 1124432296273551722
2023-07-11T00:04:00.468000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_banbrcm1
2023-07-11T00:04:00.472000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/abubakarsadiq/Desktop/bitcoin/test/functional/test_framework/test_framework.py", line 550, in start_n
...

💬 ismaelsadeeq commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259010439)
The v19 test pubkeys `0296b538e853519c726a2c91e61ec11600ae1390813a627c66fb8be7947be63c52` and `037211a824f55b505228e4c3d5194c1fcfaa15a456abdf37f9b9d97a4040afc073` are they relevant to the test? I see they are the same as the one reported in #18075

💬 ismaelsadeeq commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259015544)
Why are we no longer using the wallet?

💬 ismaelsadeeq commented on pull request "test: Fixes and updates to wallet_backwards_compatibility.py for 25.0 and descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/28027#discussion_r1259013655)
`180000`, `210000`, e.t.c
To reduce the magic numbers we can create a variable for the versions, e.g `v18 = 180000`and reuse it throughout the tests?