💬 dergoegge commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186101443)
> happy to take suggestions.

I think it would make sense for the `opencon` thread (`ThreadOpenConnections`) to handle this.

> What do you suggest to use instead of block height?

```c++
if (m_last_asmap_health_check + 24h < NodeClock::now()) {
// Do health check
}
```

👍 dergoegge approved a pull request: "fuzz: BIP 42, BIP 30, CVE-2018-17144"
(https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1414856320)
Code review ACK fa2d8b61f9343d350b67357a12f39b613c8ee8ad

👍 pablomartin4btc approved a pull request: "Wallet : Allow user to navigate options while encrypting at creation"
(https://github.com/bitcoin-core/gui/pull/722#pullrequestreview-1414865992)
re-ACK 78660e72001a2561c7ad3026367a69f65414dbd9. cc @jarolrod.

I see[`wallet_create_tx.py --descriptors`](https://cirrus-ci.com/task/6164029874372608?logs=functional_tests#L2752)test is failing and you are re-running it, I don't think it's related with your change but let's see.

💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1536319288)
Thank you for the review @MarcoFalke and @ryanofsky

Updated 9033dc6827cc623f1f7176fde120229f36ff5e74 -> e056d6f758382d3418682095ab02f8d487aa641f ([removeBlockstorageArgs_19](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_19) -> [removeBlockstorageArgs_20](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_20), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_19..removeBlockstorageArgs_20))
* Dropped the commit changing t
...

💬 RandyMcMillan commented on issue "deadlock with wrong system date/time and non-empty wallet":
(https://github.com/bitcoin-core/gui/issues/643#issuecomment-1536322254)
I vaguely remember finding a solution to this... I will try to find it - buried in a comment somewhere in bitcoin/bitcoin issues.

If I recall - the fix had to do with (re)casting the time back to int when passing it to the gui. The gui wasnt handling the chronos? variable correctly - it was likely a bug in the QT gui framework itself. I wasnt able to "prove" it because the crash didnt produce a stack error (for the gui) - but it worked. :)

I will try repost the fix with some tests to prov
...

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin-gui -ipcconnect option":
(https://github.com/bitcoin/bitcoin/pull/19461#issuecomment-1536369001)
Thanks Sjors, I'm trying to reproduce the shutdown error but didn't succeed yet. Will try again with a mainnet node. I didn't look into the external signer bug yet, but maybe that is more straightforward. In both cases running with -debug=ipc could make it clearer what's going on, especially in the shutdown case. Probably what is happening in that case is that some thread is trying make an IPC call after the socket is closed, so an ipc::Exception is raised which is uncaught. Fix might be synchro
...

💬 jamesob commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536375432)
It seems inevitable to me that well-coordinated "third party" mempools will emerge which eschew all policy and use out-of-band payment as the only means of DoS resistance. Seems likely these sources will be paid attention by large mining pools. I'm sympathetic to the motivations behind this change on those grounds, as well as with the general frustration around the sharp edges of policy (e.g. pinning).

The notion of having to lobby a council, however qualified, to "accept a usecase" is also u
...

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1186200624)
unrelated: The following diff compiles for me:

```diff
diff --git a/src/kernel/chainstatemanager_opts.h b/src/kernel/chainstatemanager_opts.h
index 2395f60164..dd75985332 100644
--- a/src/kernel/chainstatemanager_opts.h
+++ b/src/kernel/chainstatemanager_opts.h
@@ -29,7 +29,7 @@ namespace kernel {
*/
struct ChainstateManagerOpts {
const CChainParams& chainparams;
- fs::path datadir;
+ const fs::path datadir;
const std::function<NodeClock::time_point()> adjusted_t
...

🤔 pablomartin4btc reviewed a pull request: "net: Continuous ASMap health check"
(https://github.com/bitcoin/bitcoin/pull/27581#pullrequestreview-1414971325)
Concept ACK.

💬 pablomartin4btc commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1186205062)
I think you need to declare `addr` as `const` (`tidy`CI test is failing due to that).
```suggestion
for (const auto &addr : clearnet_addrs) {
```

👍 MarcoFalke approved a pull request: "doc: Add post branch-off note about fuzz input pruning"
(https://github.com/bitcoin/bitcoin/pull/27574#pullrequestreview-1414977410)
lgtm

🤔 stickies-v reviewed a pull request: "test: Explicitly specify directory where to search tests for"
(https://github.com/bitcoin/bitcoin/pull/27561#pullrequestreview-1414950382)
Would it be possible to provide instructions on replicating how to make this fail without`sys.path.append(tests_dir)`? Often times, messing with `system.path` is treating the symptoms when actually the package structure needs to be improved. May not be true in this case, though.

💬 stickies-v commented on pull request "test: Explicitly specify directory where to search tests for":
(https://github.com/bitcoin/bitcoin/pull/27561#discussion_r1186191720)
Found two more instances in test_runner.py:

```diff
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index 0c9a7173c..d495615fc 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -412,7 +412,7 @@ def main():

# Read config generated by configure.
config = configparser.ConfigParser()
- configfile = os.path.abspath(os.path.dirname(__file__)) + "/../config.ini"
+ configfile = os.path.abspath(os.path.join(os.p
...

💬 theuni commented on pull request "Enable HW-accelerated implementations of SHA256 for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/24773#discussion_r1186221811)
Hmm, ok, we are talking around each-other. I'll ask some detailed questions in code and see if we can clear it up.

💬 theuni commented on pull request "Enable HW-accelerated implementations of SHA256 for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/24773#discussion_r1186222219)
How does this not get defined twice? Does MSVC not define `__x86_64__` or `__amd64__` or `__i386__` ?

💬 theuni commented on pull request "Enable HW-accelerated implementations of SHA256 for MSVC builds":
(https://github.com/bitcoin/bitcoin/pull/24773#discussion_r1186223293)
Why does the existing asm function not work? What is the specific error? Does it not like the inline asm? Or does it just have a problem with the opcode?

💬 ishaanam commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1186226669)
Done

💬 ishaanam commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1186226878)
Done

💬 ishaanam commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1186227089)
Done

💬 ishaanam commented on pull request "rpc: add `descriptorprocesspsbt` rpc":
(https://github.com/bitcoin/bitcoin/pull/25796#discussion_r1186230858)
I've rewritten the test to reflect this. I've also added more comments.

💬 instagibbs commented on pull request "Allow accepting non-standard transactions on mainnet":
(https://github.com/bitcoin/bitcoin/pull/27578#issuecomment-1536428552)
> The notion of having to lobby a council, however qualified, to "accept a usecase" is also understandably unappealing at face value.

This is why *rdinals haven't been seriously curtailed(or even considered in this repo); taproot was designed with specific goals in mind to allow people to do whatever they want, as long as they aren't causing systemic issues with relay/validation/miners.

On the other hand, legacy script is full of DoS disasters(that we can't simply softfork out because lit
...