💬 RCasatta commented on pull request "rpc: Optimize serialization disk space of dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1472285629)
Yeah as I specified in the issue and as @TheCharlatan wrote more ram shouldn't be needed because txid are iterated sorted from leveldb.

I was also wondering why the `bitcoin-cli savetxoutset` API (same goes for `savemempool`) requires to specify a filename instead of writing to stdout that would offer better composition while maintaining the possibility to write to file with `>utxo.bin`

📝 hebasto converted_to_draft a pull request: "refactor: Use move semantics instead of custom swap functions"
(https://github.com/bitcoin/bitcoin/pull/26749)
This PR makes code more succinct and readable by using move semantics.

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1139007798)
Well, the [reason](https://api.cirrus-ci.com/v1/task/5641079790239744/logs/ci.log) of that change is to avoid using of `vChecks` after it was moved:
```
test/checkqueue_tests.cpp:166:13: error: 'vChecks' used after it was moved [bugprone-use-after-move,-warnings-as-errors]
vChecks.resize(std::min(total, (size_t) InsecureRandRange(10)));
^
test/checkqueue_tests.cpp:168:21: note: move occurred here
control.Add(std::move(vChecks));
^
`
...

👋 hebasto's pull request is ready for review: "refactor: Use move semantics instead of custom swap functions"
(https://github.com/bitcoin/bitcoin/pull/26749)

📝 ismaelsadeeq opened a pull request: "test: Support decoding segwit address in address_to_scriptpubkey()"
(https://github.com/bitcoin/bitcoin/pull/27269)
[rpc_scantxoutset.py](https://github.com/bitcoin/bitcoin/blob/e695d8536e534f1e59de4a62a0d87a93e7a73456/test/functional/rpc_scantxoutset.py#L26) sendtodestination only sends to legacy addresses and scriptPubkeys because [wallet.py](https://github.com/bitcoin/bitcoin/blob/e695d8536e534f1e59de4a62a0d87a93e7a73456/test/functional/test_framework/wallet.py#L415) address_to_scriptpubkey does not support conversion of segwit address.

This update enables address_to_scriptpubkey to support the conver
...

💬 hebasto commented on issue "Add support for sighash flags in PSBT (like SINGLE|ANYONECANPAY)":
(https://github.com/bitcoin/bitcoin/issues/27141#issuecomment-1472352358)
> This is a duplicate for [bitcoin-core/gui#712](https://github.com/bitcoin-core/gui/issues/712) and can be closed

Let's continue the discussion in https://github.com/bitcoin-core/gui/issues/712.

✅ hebasto closed an issue: "Add support for sighash flags in PSBT (like SINGLE|ANYONECANPAY)"
(https://github.com/bitcoin/bitcoin/issues/27141)

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1472365761)
In the last push i've:
- Added a commit to avoid a stack overflow during the destruction of a too large `Node`, due to recursive calls in `shared_ptr`'s destructor. (The reason the CI was failing under MSVC.)
- Lowered the maximum size of a TapMiniscript to make sure a spending witness would never hit the maximum standard transaction size limit. (It's still pretty high.)
- Addressed all comments from @bigspider.

This PR is now ready for another round of review.

💬 MarcoFalke commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1139075963)
bug fixes should be separate from refactoring commits

💬 0xB10C commented on pull request "mempool: Add mempool tracepoints":
(https://github.com/bitcoin/bitcoin/pull/26531#issuecomment-1472375220)
ACK d0c6da92c5a4d2a972f884f29dd716da2040fdb2

Code reviewed the mempool tracepoint interface tests and the `mempool_monitor.py` example script including the BCC code for both. Tracepoint placement also looks good and the tracepoint arguments match whats added in the documentation.

Two things to note:
- the `mempool_monitor.py` demo script will, over time, use more and more system resources (RAM and CPU) as mempool events are added to the `events` list. The list is never cleared. The entrie
...

💬 1440000bytes commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1472399681)
Concept ACK

Some questions:

> Previously a transaction containing an OP_RETURN without data would be rejected if datacarrier was set to false, or the datacarriersize was set to zero.

Minimum for `datacarriersize` is 1 so tx shouldn't be rejected if `datacarrier` is also 1(default)?

> All this pull-req changes is it'll allow a bare OP_RETURN in the (rare) case that people are disabling datacarrier outputs, because a bare OP_RETURN carries no data.

It is still OP_RETURN output, righ
...

💬 instagibbs commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1472404739)
Honestly I think it's time to discuss removing the `datacarrier` option altogether. Less code and args being threaded through for something that no one sets(?), especially in light of tapscript witness sizes.

📝 dergoegge opened a pull request: "refactor, net processing: Avoid CNode::m_relays_txs usage"
(https://github.com/bitcoin/bitcoin/pull/27270)
`CNode::m_relays_txs` is meant to only be used for the eviction logic in `net`. `TxRelay::m_relay_txs` will hold the same value and is meant to be used on the application layer to determine if we will/should relay transactions to a peer.

(Shameless plug: we should really better specify the interface for updating eviction data to avoid refactors like this in the future -> #25572)

💬 dergoegge commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1139046536)
```suggestion
if (m_txreconciliation) m_txreconciliation->TryRemovingFromSet(pfrom.GetId(), wtxid);
```

You are already checking internally that the peer is registered.

💬 dergoegge commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#discussion_r1139078818)
```suggestion
{
LOCK(m_peer_mutex);
for (auto [id, peer] : m_peer_map) {
const auto state{State(id)};
if (!state) continue;

if (auto tx_relay = peer->GetTxRelay()) {
LOCK(tx_relay->m_bloom_filter_mutex);

...

📝 hernanmarino opened a pull request: "Wallet : Allow user to navigate options while encrypting at creation"
(https://github.com/bitcoin-core/gui/pull/722)
This fixes https://github.com/bitcoin-core/gui/issues/394
It adds a "Go back" button to th "Confirm wallet encryption" window, allowing the users to change the password if they want to. It also adds a Cancel button to "Wallet to be encrypted window"
Prior to this change users had no option to alter the password, and were force to either go ahead with wallet creation or cancel the whole process. Also, at the final window, they were shown a Warining but with no option to cancel.
The new workf
...

💬 hebasto commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1139144192)
> bug fixes should be separate from refactoring commits

Commits have been reorganized.

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1472457091)
Updated changes:
- move the validation/parsing logic up in the stack as soon as the request is initiated even before the work item thread is created
- updated unit and functional tests

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139019170)
nit:

```suggestion
const std::optional<CNetAddr> addr{LookupHost(str_addr, /*fAllowLookup=*/false)};
```

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139037448)
Hmm, `std::optional` without the template parameter? I guess the compiler is smart enough to deduct it, but in other places you used `std::optional<CNetAddr>`, better add the template parameter here for consistency. Or use `const auto addr = ...` eveywhere?

💬 vasild commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1139005096)
This change in the comment about `Lookup()` was done in commit
```
5a656f60fb p2p, refactor: return `std::vector<CNetAddr>` in `LookupHost`
```
but it belongs to the next commit
```
786ad94614 p2p, refactor: return vector/optional<CService> in `Lookup`
```