💬 MarcoFalke commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1519637810)
Fuzzing segfaults, see CI

💬 MarcoFalke commented on pull request "test: simplify uint256 (de)serialization routines":
(https://github.com/bitcoin/bitcoin/pull/27516#issuecomment-1519661945)
lgtm ACK 96bf0bca4a0e3aa0b7c07d8c225861e72f970fa9

💬 MarcoFalke commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1174994067)
You'll have to do it in this pull. Otherwise CI will warn: E: The repository 'https://ppa.launchpadcontent.net/hadret/bpfcc/ubuntu lunar Release' does not have a Release file.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1519869383)
> Fuzzing segfaults, see CI

Saw it yesterday, working on it, thanks.

👍 brunoerg approved a pull request: "test: simplify uint256 (de)serialization routines"
(https://github.com/bitcoin/bitcoin/pull/27516#pullrequestreview-1397816417)
crACK 96bf0bca4a0e3aa0b7c07d8c225861e72f970fa9

📝 FlannelDipole opened a pull request: "Updated the installation instructions for macOS"
(https://github.com/bitcoin/bitcoin/pull/27525)
Added: Instructions on changing directories after the repo clone (cd bitcoin)
(Rationale: The initial documentation assumes that the user knows to change the current directory to the cloned repository before running further commands. However, for new users or those unfamiliar with the command line, this step might not be obvious.)

Added: Instructions on verifying the build was successful and the GUI was compiled.
(Rationale: After compiling the Bitcoin Core software, it's important to verif
...

💬 ishaanam commented on pull request "bumpfee: allow send coins back to yourself":
(https://github.com/bitcoin/bitcoin/pull/27195#issuecomment-1520235843)
reACK be72663a1521bc6cdf16d43a4feae7c5b57735c0

💬 satsie commented on issue "Consider Removing Message Signing":
(https://github.com/bitcoin/bitcoin/issues/27515#issuecomment-1520380907)
Given that all features require some effort to maintain, does this message signing code have a disproportionately high maintenance cost? You mention it is being abused to verify wallets, but I want to echo jlopp's comment about its usage being, in the absence of another well established standard (official or not), a way to arbitrarily sign messages with private keys. From what I have seen, this feature is mainly used to verify the integrity of hardware and software wallets--to make sure the BIP-
...

💬 jonatack commented on pull request "Remove BIP35 mempool p2p message":
(https://github.com/bitcoin/bitcoin/pull/27426#issuecomment-1520408121)
Linking here this response to the ML request for feedback.

https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2023-April/021563.html

💬 ekzyis commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1520485069)
utACK d8434da3c14ed6723d86ef2cd266008d366e1413

> **If we want to allow for more configurable signet consensus rules**, then I think we need to do it in a more robust way rather than ad-hoc adding options that all users must remember to set if they want to use a particular signet. As these are consensus parameters, forgetting to set the option correctly will result in eventual consensus failure which is generally only resolved by nuking the data directory. This may garner additional and unnece
...

⚠️ darosior opened an issue: "wallet coin selection: don't mixup coins with absolute timelocks of different types"
(https://github.com/bitcoin/bitcoin/issues/27526)
We now support spending from coins containing `CHECKLOCKTIMEVERIFY` instructions. `CLTV` is a check on the value of the `nLockTime` field of the spending transaction. There are two types of timelocks possible: by timestamp and by block. Both are incompatible, the `nLockTime` field cannot satisfy both a timestamp and a block timelock.

The coin selection is at the moment completely unaware of timelocks. If we have two coins for two different descriptors, one with a timestamp absolute timelock a
...

⚠️ darosior opened an issue: "wallet coin selection: be aware of timelocks and allow commands to set an optional block height when selecting coins"
(https://github.com/bitcoin/bitcoin/issues/27527)
The wallet should not select a yet-unavailable coin to fund a transaction. We could use the latest block height by default and allow the user to set one when calling for instance `walletcreatefundedpsbt`, to effectively tell the wallet "select me coins such as they'll be available after block height `N`).

Also maybe the wallet should set the right values for `nSequence` / `nLockTime` if it spends timelocked coins? Curious what people think.

cc @ishaanam since we discussed it together.

💬 benthecarman commented on issue "-fallbackfee should apply to estimatesmartfee":
(https://github.com/bitcoin/bitcoin/issues/27415#issuecomment-1520674421)
+1 this would be nice, applications like CLN use this rpc and makes it hard to test in regtest / signet environments

📝 theStack opened a pull request: "test: fix `feature_addrman.py` on big-endian systems"
(https://github.com/bitcoin/bitcoin/pull/27529)
The test `feature_addrman.py` currently serializes the addrdb without specifying endianness for `int`s, so the machine's native byte order is used (see https://docs.python.org/3/library/struct.html#byte-order-size-and-alignment) and the generated `peers.dat` would be invalid on big-endian systems (our internal (de)serializers always use little-endian, see `ser_{read,write}data32`). Fix this by explicitly specifying little-endian serialization via the `<` character in `struct.pack(...)`.

This
...

💬 MarcoFalke commented on pull request "test: fix `feature_addrman.py` on big-endian systems":
(https://github.com/bitcoin/bitcoin/pull/27529#issuecomment-1521387810)
There is `ci/test/00_setup_env_s390x.sh`, but I guess it runs qemu-user, not qemu-system, so the python part will run on the host endianness. I wonder how hard it would be to spin up qemu-system in the CI env?

💬 achow101 commented on pull request "p2p: skip netgroup diversity follow-up":
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1521500955)
While renaming this variable is technically correct, I don't think it's useful to have these kind of PRs. Although it is small and trivial, it does take away time from review, has an effect on git blaming, and possibly conflicts with other PRs in this area that do introduce meaningful changes.

💬 jonatack commented on pull request "p2p: skip netgroup diversity follow-up":
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1521526214)
@achow101 The code comment needs to be fixed as it is now incorrect, and the naming is now misleading. I think the change is clearly justified here.

💬 MarcoFalke commented on pull request "Fix unsigned integer overflows in interpreter":
(https://github.com/bitcoin/bitcoin/pull/24214#issuecomment-1521549361)
Closing for now due to inactivity. I still think this is useful (see OP) and we should re-evaluate it the next time the interpreter.cpp is changed to catch newly introduced unintended unsigned integer overflow.

✅ MarcoFalke closed a pull request: "Fix unsigned integer overflows in interpreter"
(https://github.com/bitcoin/bitcoin/pull/24214)

🚀 fanquake merged a pull request: "test: simplify uint256 (de)serialization routines"
(https://github.com/bitcoin/bitcoin/pull/27516)

💬 jonatack commented on pull request "refactor: move index class members from protected to private":
(https://github.com/bitcoin/bitcoin/pull/24150#issuecomment-1521605660)
Closing temporarily so that I can re-open it -- am still interested to work on this.