🚀 fanquake merged a pull request: "build: Require C++20 compiler"
(https://github.com/bitcoin/bitcoin/pull/28349)

⚠️ maflcko opened an issue: "test: Intermittent issue in rpc_net.py"
(https://github.com/bitcoin/bitcoin/issues/29030)
https://drahtbot.space/temp_scratch/rpc_net_1408.tar.xz

```
test 2023-12-08T12:02:27.339000Z TestFramework (INFO): Check getpeerinfo output before a version message was sent
node0 2023-12-08T12:02:27.341389Z [http] [httpserver.cpp:306] [http_request_cb] [http] Received a POST request for / from 127.0.0.1:55486
node0 2023-12-08T12:02:27.342184Z [httpworker.0] [rpc/request.cpp:187] [parse] [rpc] ThreadRPCServer method=setmocktime user=__cookie__
test 2023-12-08T12:02:27.346000Z Tes
...

💬 maflcko commented on pull request "wallet: batch all individual spkms setup db writes in a single db txn":
(https://github.com/bitcoin/bitcoin/pull/28894#discussion_r1420361508)
question: Does it need to be random? `m_path_root` should already be (more) random. If not, it would be good to remove. Also, to reduce the use of `c_str`, which (in other parts of the code) is fragile and can lead to bugs. So `git grep c_str` will be less verbose if unused used of c_str are removed.

💬 maflcko commented on pull request "test: doc: follow-up #28368":
(https://github.com/bitcoin/bitcoin/pull/29013#issuecomment-1847079707)
```
e-argument-comment,-warnings-as-errors]
52 | /*m_mempool_limit_bypassed=*/false,
| ^
./kernel/mempool_entry.h:256:51: note: 'from_disconnected_block' declared here
256 | const bool from_disconnected_block, const bool submitted_in_package,
| ^
test/fuzz/poli
...

🤔 josibake reviewed a pull request: "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction"
(https://github.com/bitcoin/bitcoin/pull/25273#pullrequestreview-1772112219)
ACK https://github.com/bitcoin/bitcoin/pull/25273/commits/1d1a6ff03215cf3aadd5fd1118f354b1ba8ffa5a

Left some nits that can be ignored or addressed in a follow-up, but nothing major. There is a type mismatch with `GetInputWeight` returning int64_t but being assigned to int32_t. Would be great to fix that but also not a super big deal since weight will never be greater than int32_t anyway (but should def be fixed in a followup).

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420275230)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

nit: kind of annoying that we call this an `output` but below in `GetExternalOutput` we (correctly) call it an `outpoint`, would be nice to use consistent naming if this ever gets retouched.

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420279769)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

nit: `s/output/outpoint/`

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420316907)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

`GetInputWeight` is currently returning `int64_t` but I think `int32_t` is the more appropriate choice given that this represents weight.

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420318483)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

same as above, type mismatch between `int32_t` and what's returned, `int64_t`

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420293648)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

nit: `s/out/outpoints/`, in this case I think it helps make it more clear why you are doing the the transform: we are returning just the outpoints from the `m_selected` vector

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420333328)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

`int32_t`

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420306769)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/7b5d22f44f515b9e6ab301ca4cb2a155796e6ebc:

Is there a situation where an internal input would have a value for `TxOut`, accidentally or purposefully? Only asking because it feels _slightly_ more fragile to rely only on this to determine if something is external or internal vs having two separate vectors, but also don't have a good sense if what I'm worried about is even possible (internal having `TxOut` set) or if its even that bad if we
...

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420348354)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/2a06c4c2b33c614cc3f877bdc013c8df9e847ca9:

in earlier commits, you were replacing the "Has -> Get" pattern with a "Get" that returned a std::optional. Why can't we also do that here?

💬 josibake commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420332476)
in https://github.com/bitcoin/bitcoin/pull/25273/commits/5ce3008d46c12f37ac4ea19d920df3464fe704d8:

shouldn't this be `int32_t`?

💬 fanquake commented on issue "bitcoin 25.1 regression test failure against sqlite 3.44.1":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1847113188)
This is happening again in https://github.com/Homebrew/homebrew-core/pull/156745.
i.e https://github.com/Homebrew/homebrew-core/actions/runs/7141174823/job/19448089734?pr=156745#step:4:47.

💬 0xB10C commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847121311)
post-merge ACK. Tested this on my NixOS setup. Configure and build works fine, unit tests pass. Reverting in faa48388bca06df1ca7ab92461b76a6720481e45 makes sense.



<details>
<summary>
I see a few `-Wmaybe-uninitialized` warnings not sure if they're related.
</summary>

```
CXX policy/libbitcoin_common_a-feerate.o
In file included from ./hash.h:13,
from ./pubkey.h:10,
from ./addresstype.h:9,
from ./outputtype.h:9,

...

💬 dergoegge commented on issue "fuzz: Left over tmp files when fuzzing with afl++":
(https://github.com/bitcoin/bitcoin/issues/28811#issuecomment-1847123265)
* we could (but probably won't) refactor our code so that we don't need to go to disk at all.
* or use a custom signal handler for e.g. SIGUSR1 that deletes the data dir and then exits without any other cleanup. Afl++ can be told to send any signal instead of SIGKILL to children on timeout, etc with `AFL_KILL_SIGNAL`.

I've worked around this so far by using a ram disk and fuzzing inside a docker container, once the disk is full the whole thing just dies and I can inspect the container afterw
...

🤔 BrandonOdiwuor reviewed a pull request: "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs"
(https://github.com/bitcoin/bitcoin/pull/28979#pullrequestreview-1772293975)
Concept ACK

⚠️ maflcko reopened an issue: "bitcoin 25.1 regression test failure against sqlite 3.44.1"
(https://github.com/bitcoin/bitcoin/issues/28941)
While upgrading sqlite to 3.44.1, we've found some regression test failure


<details>
<summary>test failure log</summary>

```
==> /home/linuxbrew/.linuxbrew/Cellar/bitcoin/25.1/bin/test_bitcoin
Running 557 test cases...
test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [3933824172291540 != 0]
test/serfloat_tests.cpp(96): error: in "serfloat_tests/double_serfloat_tests": check v == v2 has failed [1522707584769707 != 0]
t
...

💬 maflcko commented on issue "bitcoin 25.1 regression test failure against sqlite 3.44.1":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1847146801)
Again, it would be good to have steps to reproduce locally.

Also, `double_serfloat_tests` has nothing to do with sqlite. (You can compile them without wallet or sqlite)