💬 schjonhaug commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1560053023)
Looks like `>>` is missing after `"maxconnections=2"` in

```
$ cd $DATA_DIR && echo "signet=1" >> bitcoin.conf && echo "maxconnections=2" bitcoin.conf && cd ~
```

Also, it’s easier to copy the commands using the (hover) copy icon <img width="46" alt="image" src="https://github.com/bitcoin/bitcoin/assets/84225/ea868344-91f5-4e4b-b82c-5628f05a88bc"> on Github if you remove the `$` in front.

💬 theuni commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1202937682)
Would it be possible to keep pointers/references to internal data out of this interface from the beginning? I'm not sure if that's a goal at this layer.

For blocktip for example, taking a quick look around, for the ui it looks like this eventually gets converted to:
`void(SynchronizationState, interfaces::BlockTip tip, double verification_progress)`

I wonder if we can do the same here?

🤔 theuni reviewed a pull request: "kernel: Remove interface_ui, util/system from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1440486632)
From IRC:

> <TheCharlatan> Hey, how about moving the last two commits of #27636 (the only ones to touch the shutdown logic) to #27711? Then we can get #27636 merged and move ahead with #27576.

This is a good idea imo. I really don't want to slow this down, but I would benefit from a few extra days of catching up and testing the shutdown logic before reviewing/critiquing it in detail.

📝 fjahr opened a pull request: "init: Improve file descriptor limit handling"
(https://github.com/bitcoin/bitcoin/pull/27730)
I was playing with file descriptor limits to test another issue saw the following warning printed when I had configured a file descriptor limit of 150:

```
Warning: Reducing -maxconnections from 125 to -9, because of system limitations.
```

The warning could have been resolved with a one-line change but I found the surrounding code hard to reason about at first, so I added a few comments and reorganized the code a bit for better clarity.

Aside from the warning there is one further beh
...

💬 schjonhaug commented on issue "v25.0 testing":
(https://github.com/bitcoin/bitcoin/issues/27621#issuecomment-1560102597)
A comment on the following paragraphs:

> To create a non-witness transaction of 64 bytes or so, You can generate an OP_RETURN output with the following dummy data in hexadecimal format: 6a00000000.

> Get a new change address
> $ bcli -regtest getnewaddress
> You should create a transaction with two outputs: one that sends Bitcoin to the address you generated, and the other that is an OP_RETURN output with the hex value of 6a00000000

> $ bcli -regtest createrawtransaction '[{"txid":"04
...

💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1560143021)
Updated per feedback, thanks @ishaanam.

💬 furszy commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1203034031)
pushed

💬 achow101 commented on pull request "Improve display address handling for external signer":
(https://github.com/bitcoin/bitcoin/pull/24313#discussion_r1203035210)
In 8218ce6b02b24eeef31f4e343ea777b7844907fc "wallet: return and display signer error"

Returning the error in an optional feels a bit wrong since it inverts are typical pattern. Rather than that the result is equivalent to `true`, we have to check that it's equivalent to `false`.

I think it would be better to use `util::Result<bool>` instead of an optional in order to preserve our typical error checking pattern.

💬 achow101 commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#issuecomment-1560158824)
ACK 76396ca376188631ba46bd47b134881efcc6f755

👍 brunoerg approved a pull request: "rpc: Fix invalid bech32 address handling"
(https://github.com/bitcoin/bitcoin/pull/27727#pullrequestreview-1440618244)
crACK eeee55f9288740747b6e8d806ce8177fd92635cf

💬 brunoerg commented on pull request "rpc: Fix invalid bech32 address handling":
(https://github.com/bitcoin/bitcoin/pull/27727#discussion_r1203063730)
feel free to ignore:

since it's just 1 node
```suggestion
self.extra_args = [["-prune=899"]]
```

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1203079320)
Primarily, I did not want to mess with existing interfaces, but I checked through everything and it would be feasibly to entirely drop usage of `CBlockIndex` for this notification. The change is a bit sprawling though and also ends up touching rpc code.

> I'm not sure if that's a goal at this layer.

The way I see this play out during the next phase of exploring a more suitable kernel api, these notifications might be used to feed back into the `ChainstateManager`. As long as the chainman
...

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1560199240)
Dropped bbfbcd0360e486d47025a412f2c0f4e8535ab255 -> 7d3b35004b039f2bd606bb46a540de7babdbc41e ([chainstateRmKernelUiInterface_12](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_12) -> [chainstateRmKernelUiInterface_13](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_13), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_12..chainstateRmKernelUiInterface_13).

This reverts the last two commits touc
...

🤔 mzumsande reviewed a pull request: "index: prevent race by calling 'CustomInit' prior setting 'synced' flag"
(https://github.com/bitcoin/bitcoin/pull/27720#pullrequestreview-1440641924)
Code review ACK 3126454dcfa1dd29bb66500d5f2b5261684d6c58

📝 fjahr opened a pull request: "Prevent file descriptor exhaustion from too many RPC calls"
(https://github.com/bitcoin/bitcoin/pull/27731)
Fixes #11368

This requires libevent 2.2.1 which so far was only [released in alpha](https://github.com/libevent/libevent/releases/tag/release-2.2.1-alpha).

For more context see specifically https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-632717903, this uses the feature mentioned there as libevent#592.

💬 fjahr commented on issue "bitcoin core crashes when too many rpc calls are made":
(https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-1560253321)
> 2.2.1-alpha is [now out](https://github.com/libevent/libevent/releases/tag/release-2.2.1-alpha) 👀

I have opened a draft PR #27731 that uses the proposed solution with the max connection feature that was added there. But currently I still have a hard time reproducing the issue in the same way I was able to back in 2020 described above: https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-638770701 I have a much faster machine now but there may also be other reasons.

I also tried
...

⚠️ fjahr opened an issue: "Creating too many wallets exhausts file descriptor limit and leads to crash"
(https://github.com/bitcoin/bitcoin/issues/27732)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

This behavior was first noted by @EthanHeilman here: https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-751574945 but it was conflated with the RPC issue there while I now think this is a separate issue.

When a lot of wallets are created in quick succession, sqlite will exhaust all the available file descriptors and the node crashes the next time it tries to create file for un
...

💬 fjahr commented on issue "bitcoin core crashes when too many rpc calls are made":
(https://github.com/bitcoin/bitcoin/issues/11368#issuecomment-1560260664)
See #27732 for the wallet issue, I that makes sense to keep these things separate.

📝 theStack opened a pull request: "test: refactor: introduce `generate_keypair` helper with WIF support"
(https://github.com/bitcoin/bitcoin/pull/27733)
In functional tests it is a quite common scenario to generate fresh elliptic curve keypairs, which is currently a bit cumbersome as it involves multiple steps, e.g.:

privkey = ECKey()
privkey.generate()
privkey_wif = bytes_to_wif(privkey.get_bytes())
pubkey = privkey.get_pubkey().get_bytes()

Simplify this by providing a new `generate_keypair` helper function that returns the private key either as `ECKey` object or as WIF-string (depending on the boolean `wif` parameter)
...

⚠️ Sokhanetaze80 opened an issue: "Is there any documentation on PROTOCOL_VERSION I can refer to please? How is this decided and how does it cope with branches?"
(https://github.com/bitcoin/bitcoin/issues/27734)
Is there any documentation on PROTOCOL_VERSION I can refer to please? How is this decided and how does it cope with branches?

_Originally posted by @rebroad in https://github.com/bitcoin/bitcoin/pull/932#discussion_r580883_