💬 TheCharlatan commented on pull request "guix: use python-minimal (3.9)":
(https://github.com/bitcoin/bitcoin/pull/27361#issuecomment-1489001205)
ACK d0e571ebb187

```
find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
53aa12b7fe92d92eaf81a51a8f5d534f92a2f18f0218f29fdf3221d817a53c04 guix-build-d0e571ebb187/output/aarch64-linux-gnu/SHA256SUMS.part
01467530b06b90114561e86968710250e3648fe4aba5c228781ccf9ac72394c2 guix-build-d0e571ebb187/output/aarch64-linux-gnu/bitcoin-d0e571ebb187-aarch64-linux-gnu-debug.tar.gz
fdf6dfc901e371b4e11a7f69eb2ff9b36a3ac393255862bad1dcd45b
...

💬 jonatack commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1489034354)
> I found a few other similar mechanisms in other modules, dunno if this change should apply to them as well

Circling back for completeness to cover these two questions.

> https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/validation.cpp#L2565-L2569

This code appends warning messages to the `UpdateTipLog()` logging and looks correct.

> https://github.com/bitcoin/bitcoin/blob/82793f1984911774b111117f2e81d5f3b0bbec68/src/warnings.cpp#L55-L57

The `<h
...

👍 Ayush170-Future approved a pull request: "ci: cleanup of CI_EXEC & CI_EXEC_ROOT"
(https://github.com/bitcoin/bitcoin/pull/27333)

💬 furszy commented on pull request "wallet, tests: Expand and test when the blank wallet flag should be un/set":
(https://github.com/bitcoin/bitcoin/pull/25634#issuecomment-1489236967)
Before the text; this is not a blocking comment. Purely about adding some ideas.

> I'm still slightly hesitating though. One question that still bothers me is whether we should expose `blank` flag at all. It's an implementation detail and we only need it for the functional tests. It doesn't seem like there is any observable by the user behaviour of the flag, so why test it with functional tests? Unit tests are better fit for testing internal invariants.

Hmm, I also agree that exposing an i
...

💬 arnabnandikgp commented on issue "gen-manpages output depends on build options, so needs to check them":
(https://github.com/bitcoin/bitcoin/issues/17506#issuecomment-1489328037)
wanted to know whether this issue is still active as i am interested in this issue..

💬 ProofOfKeags commented on issue "Manual-pruning cursor rewind":
(https://github.com/bitcoin/bitcoin/issues/19807#issuecomment-1489330250)
> serving old blocks to bootstrapping nodes

a node should just serve the blocks it has. The cursor rewind should cause it to download earlier blocks as if it was doing IBD only it doesn't have to do any consensus validation except that the block data matches the expected blockhash

> wallet rescans

A rescan has to go back to the wallet birthday irrespective of the pruning cursor.

One other thing to note. I no longer personally need this feature, but I'm simply describing the need as I
...

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#issuecomment-1489476165)
@hebasto Thanks for the help for getting the CI/CD fixed. The fact that the version of libevent was being pinned to the earlier version explains why testing didn't catch this.

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#discussion_r1152592590)
Using the latest version of libevent in CI is now fixes the build.

💬 jonatack commented on pull request "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet":
(https://github.com/bitcoin/bitcoin/pull/27279#discussion_r1152609626)
Done, thanks.

💬 ajtowns commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1489587649)
My opinion only, but it seems like it would be simpler to have the RPC not take any arguments and always output an object:

```
$ bitcoin-cli getaddrinfo
{
"ipv4": {
"new": 5,
"tried": 3
},
...
}
```

If you want a specific network, you can use jq: `bitcoin-cli getaddrinfo | jq .ipv4` ? You can also do sums this way if desired: `| jq .ipv4.new + ipv6.new`. (Having an object rather than an array makes it easy to get at the values rather than an array and h
...

💬 nopara73 commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1489662232)
@sipa sorry for the late reply, at first I was like "yes, that makes perfect sense," however an argument came to my mind - this morning during shower :) - in favor of it, so I'd like to share that.

Can't we assume miners are seeking the highest fee transactions at all times? In other words: maximize their income from fees. If we could, then adjusting `estimatesmartfee` results based on the possibility of block acceptance of the current mempool state of the node does not seem gameable to me, b
...

📝 Doodoobrown23 opened a pull request: "Merge bitcoin/bitcoin#27247: [24.x] Bump version to v24.1rc1"
(https://github.com/bitcoin/bitcoin/pull/27367)
932a609312cb7b72bf5f23112763ab933a54389c doc: add initial release notes for v24.1 (fanquake)
cc4e3158c9e7840151fe7b6209ba5f7ea5f103c3 doc: update manual pages for v24.1rc1 (fanquake)
787affb9ea04651a682acf8d09342d6e843fdd0f doc: update version in bips.md to v24.1 (fanquake)
5077e02263cf47749583f22dc2bc609878d8290a build: bump version to v24.1rc1 (fanquake)

Pull request description:

Bump the version number to v24.1rc1.
Regenerate the man pages.
Update the version number in bips.md.
Move
...

✅ fanquake closed a pull request: "Merge bitcoin/bitcoin#27247: [24.x] Bump version to v24.1rc1"
(https://github.com/bitcoin/bitcoin/pull/27367)

📝 fanquake locked a pull request: "Merge bitcoin/bitcoin#27247: [24.x] Bump version to v24.1rc1"
(https://github.com/bitcoin/bitcoin/pull/27367)
932a609312cb7b72bf5f23112763ab933a54389c doc: add initial release notes for v24.1 (fanquake)
cc4e3158c9e7840151fe7b6209ba5f7ea5f103c3 doc: update manual pages for v24.1rc1 (fanquake)
787affb9ea04651a682acf8d09342d6e843fdd0f doc: update version in bips.md to v24.1 (fanquake)
5077e02263cf47749583f22dc2bc609878d8290a build: bump version to v24.1rc1 (fanquake)

Pull request description:

Bump the version number to v24.1rc1.
Regenerate the man pages.
Update the version number in bips.md.
Move
...

💬 Mob1le commented on pull request "fuzz: Remove legacy int parse fuzz tests":
(https://github.com/bitcoin/bitcoin/pull/27344#discussion_r1152829942)
WTF

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1152831466)
I meant, that we can write only the key for `best_xpub`. Why do we need to write all the candidates as well?

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1489825161)
Approach ACK.

Doing a proper code review now. Have a question about the upgrade. What do you think we should do in the following situation?

- Create a blank wallet with master branch
- import 6 descriptors derived from the same key. `WALLET_FLAG_GLOBAL_HD_KEY` is not set and no master key
- `getxpub` reports "This wallet does not have an active xpub"
- reload the wallet. Upgrade code is triggered, it sets the flag and the master key

This will become even more confusing when/if we la
...

💬 josibake commented on pull request "test: fix intermittent issue in `feature_bip68_sequence`":
(https://github.com/bitcoin/bitcoin/pull/27177#issuecomment-1489830936)
re-ACK https://github.com/bitcoin/bitcoin/commit/f8ebcb823fc5f39adc87ea6cf079f3e72ee44f30

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1152902779)
It is not possible to pass `nullptr` as an argument to `GetQueryParameterFromUri(const evhttp_uri& uri_parsed, const std::string& key)`. This is the point. Inside the function there is no need to worry about null pointer.

The crash occurs inside the test:

```cpp
auto uri_parsed{evhttp_uri_parse(uri.data())}; // ends up nullptr
auto param_value{GetQueryParameterFromUri(*uri_parsed, key)}; // crash here, before calling GetQueryParameterFromUri().
```

The function is safe, the c
...

💬 vasild commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1152924472)
I see you have adjusted the test in the latest push, I think that is the right approach. Thanks!