💬 brunoerg commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-1875711592)
Nice one!

📝 fanquake opened a pull request: "contrib: add macho branch protection check"
(https://github.com/bitcoin/bitcoin/pull/29170)
Followup to https://github.com/bitcoin/bitcoin/pull/28459. Add a sanity check that `bti` instructions are present in the arm macho binary, similar to our x86_64 check for control flow.

Could do something similar for aarch64 linux in future, and maybe could use https://github.com/lief-project/LIEF/issues/975.

💬 TheCharlatan commented on pull request "PoC: fuzz chainstate and block managers":
(https://github.com/bitcoin/bitcoin/pull/29158#issuecomment-1875751255)
Concept ACK

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1365807093)
As `nodeServices` is incorrect after this call, it seems to make more sense to just leave it passing `nLocalServices` here and the other call site?

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1365836836)
Maybe it should be on by default?

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1365833858)
No reason to delete the commas

💬 luke-jr commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1440760716)
Passing options individually like this is ugly - worse than just passing `const ArgsManager&` IMO. If you really want to cache it as a bool, maybe make a struct for connection permission options or something to pass instead.

Another possibility is to just cache a single `NetPermissionFlags` for these.

💬 sipa commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1440772571)
I think this comment should stay, because `NodeClock` isn't guaranteed to be monotonic?

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updates comment in ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#discussion_r1440804107)
Rebased and updated the comment. I also extended the test slightly to cover all possible cases

💬 achow101 commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1440702286)
In a9d67ad96e5f4d6dca64df4ac85f0434e17248ab "test: add tests for fundrawtx and sendmany rpcs"

There are existing tests for `sendmany` in wallet_basic.py. I think these tests could be moved there as well.

💬 achow101 commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1440697948)
In a9d67ad96e5f4d6dca64df4ac85f0434e17248ab "test: add tests for fundrawtx and sendmany rpcs"

nit: More pythonic to write:

```suggestion
tx.vout = [CTxOut(1 * COIN, bytearray(address_to_scriptpubkey(address)))] * 2
```

💬 furszy commented on pull request "wallet: Migrate entire address book entries to watchonly and solvables too":
(https://github.com/bitcoin/bitcoin/pull/28610#discussion_r1440800458)
Would be good to unload the wallets at the end of the test case.

👍 furszy approved a pull request: "wallet: Migrate entire address book entries to watchonly and solvables too"
(https://github.com/bitcoin/bitcoin/pull/28610#pullrequestreview-1802823938)
Code review ACK 406b71ab


With two non-blocking points:

Firstly, made a test to verify that the transaction's extra information is preserved during migration: c1aabd1b2a. This includes the preservation of bump fee info ('replaces_txid' and 'replaced_by_txid') and the user's custom comments ('comment' and 'comment_to'). Feel free to cherry-pick it, or I can push it in a follow-up. Either way is fine for me.

Secondly, shilling mode; with #26836, which now is part of #28574, this could ha
...

💬 furszy commented on pull request "wallet: Migrate entire address book entries to watchonly and solvables too":
(https://github.com/bitcoin/bitcoin/pull/28610#discussion_r1440735716)
tiny nit: could provide the `avoid_reuse` flag to `createwallet()` instead of calling `setwalletflag` separately.

💬 brunoerg commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#discussion_r1440838577)
Couldn't we fuzz ReadFlag with a name we didn't previously write?

💬 hebasto commented on issue "`-min` does not minimize wallet loading dialog":
(https://github.com/bitcoin-core/gui/issues/748#issuecomment-1875894079)
Confirming that the issue still exists for the release/Guix built `bitcoin-qt`.

> I guess something in the way we build qt is causing this?

The static Qt in depends misses GTK+ support. However, I'm not sure if it is the cause of the issue.

---

Other issues with different behavior for release binaries:
- https://github.com/bitcoin-core/gui/issues/33
- https://github.com/bitcoin-core/gui/issues/639

👍 jonasnick approved a pull request: "Update libsecp256k1 subtree for 0.4.1 release"
(https://github.com/bitcoin/bitcoin/pull/29169#pullrequestreview-1802996733)
ACK c13a17c6996442f04635bdf70ee8f06bf6854ff6 no difference to my locally checked out version.

📝 Christewart opened a pull request: "64bit arith"
(https://github.com/bitcoin/bitcoin/pull/29171)

✅ Christewart closed a pull request: "64bit arith"
(https://github.com/bitcoin/bitcoin/pull/29171)

📝 brunoerg opened a pull request: "fuzz: set `nMaxOutboundLimit` in connman target"
(https://github.com/bitcoin/bitcoin/pull/29172)
Setting `nMaxOutboundLimit` (`-maxuploadtarget`) will make fuzz to reach more coverage in connman target. This value is used in `GetMaxOutboundTimeLeftInCycle`, `OutboundTargetReached` and `GetOutboundTargetBytesLeft`.