💬 luke-jr commented on pull request "CLI: Only one Request Handler can be specified.":
(https://github.com/bitcoin/bitcoin/pull/27815#discussion_r1240570488)
```suggestion
if (gArgs.IsArgSet("-getinfo") + gArgs.GetBoolArg("-netinfo", false) + gArgs.GetBoolArg("-generate", false) + gArgs.GetBoolArg("-addrinfo", false) > 1) {
```

This is admittedly a bit obscure, so maybe there's a better way (but it's still an improvement over the temporary variable IMO)

💬 luke-jr commented on pull request "Sanitizing ports of -rpcconnect and -rpcport.":
(https://github.com/bitcoin/bitcoin/pull/27820#issuecomment-1605231151)
Concept ACK

💬 luke-jr commented on pull request "[Silent Payments]: Base functionality":
(https://github.com/bitcoin/bitcoin/pull/27827#issuecomment-1605232308)
> Adds support to the Bitcoin Core wallet for receiving silent payments

Prefer to leave this for a second PR

💬 luke-jr commented on pull request "Ignore datacarrier limits for dataless OP_RETURN outputs":
(https://github.com/bitcoin/bitcoin/pull/27261#issuecomment-1605235521)
>We already allow transactions to pick nSequence and nLocktime,

It's not possible to omit them...

>allowing for far more bits of data than the data embedded by the presence or absence of an OP_Return.

As far as actual data usage, the additional output requires *at least* 10 bytes (64-bit amount, length of script, and the actual OP_RETURN).

💬 luke-jr commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1605237375)
Concept ACK

💬 ryanofsky commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#issuecomment-1605245391)
> Would prefer to keep refactoring and bugfixes in separate PRs, if there's no reason they have to be together here?

It's an internal bug and the only way to trigger it is to use #27596 and restart the node during background snapshot validation. The bug was introduced by code in 0fd599a51a700c028d6f7ed8067d2d9f6e6a04a5 which was basically untested and broken, and the bug is fixed by c21624c14b83e30f32d5f8c653b3e64235625780 which depends on the earlier refactoring. Some more details about it a
...

💬 jarolrod commented on pull request "Add menu option to migrate a wallet":
(https://github.com/bitcoin-core/gui/pull/738#discussion_r1240571403)
we seem to use this style now, although it's not officially documented, so only nit

```diff
diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h
index 54eb720d0..d6a92cba7 100644
--- a/src/interfaces/wallet.h
+++ b/src/interfaces/wallet.h
@@ -428,8 +428,7 @@ struct WalletTxOut
};

//! Migrated wallet info
-struct WalletMigrationResult
-{
+struct WalletMigrationResult {
std::unique_ptr<Wallet> wallet;
std::optional<std::string> watchonly_wallet_name;
s
...

💬 jarolrod commented on pull request "Add menu option to migrate a wallet":
(https://github.com/bitcoin-core/gui/pull/738#discussion_r1240573531)
nit
```suggestion
box.setStandardButtons(QMessageBox::Yes | QMessageBox::Cancel);
```

💬 jarolrod commented on pull request "Add menu option to migrate a wallet":
(https://github.com/bitcoin-core/gui/pull/738#discussion_r1240585920)
Confirmed that all the new strings introduced here are nicely transferred over to be translated after running `make translate`

While not a blocker; the longer I look at this, the more I think it can be slimmed down. Will have another look tomorrow.

At least this is a chunky boy to translate, and will need a good translator comment:
```
<message>
<location line="+1"/>
<source>Migrating the wallet will convert this wallet to one or more descriptor wallets. A new wallet ba
...

💬 laanwj commented on pull request "Add support for RNDR/RNDRRS for AArch64 on Linux":
(https://github.com/bitcoin/bitcoin/pull/26839#discussion_r1240597346)
That means the chip, or at least this feature, is broken, and should be disabled at the kernel errata level. No need for us to handle that in user space.

🤔 pablomartin4btc reviewed a pull request: "http: update libevent workaround to correct version"
(https://github.com/bitcoin/bitcoin/pull/27949#pullrequestreview-1496428056)
Concept ACK.

I've checked the 2 fixes, checked the [release-2.1.9-beta's changelog](https://raw.githubusercontent.com/libevent/libevent/release-2.1.9-beta/ChangeLog), and after downloading the [release-2.1.9-beta](https://github.com/libevent/libevent/releases/tag/release-2.1.9-beta), checked that the changes are in there too.

💬 tansanDOTeth commented on issue "Stuck in Endless Pre-Syncing Headers Loop":
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1605385608)
Is this normal? I'm looking at the logs and it looks like it happens quite often.

```
023-06-24T10:32:33Z Pre-synchronizing blockheaders, height: 775060 (~97.47%)
2023-06-24T10:32:33Z Pre-synchronizing blockheaders, height: 777060 (~97.71%)
2023-06-24T10:32:33Z Pre-synchronizing blockheaders, height: 779060 (~97.95%)
2023-06-24T10:32:33Z Pre-synchronizing blockheaders, height: 781060 (~98.19%)
2023-06-24T10:32:34Z Pre-synchronizing blockheaders, height: 783060 (~98.43%)
2023-06-24T10:32
...

💬 aleks-mariusz commented on issue "Stuck in Endless Pre-Syncing Headers Loop":
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1605387912)
> > What are the recommendations on fixing this?
>
> If your hardware doesn't have any faults, you can do a `-reindex` to wipe the corrupt block file from the storage.

This helped, re-indexing, but this throws away the entire downloaded and starts over at 0% :-/ it took 3+ days to get back to current state sadly w/ my hardware/network connection

💬 tansanDOTeth commented on issue "Stuck in Endless Pre-Syncing Headers Loop":
(https://github.com/bitcoin/bitcoin/issues/26391#issuecomment-1605388228)
> > > What are the recommendations on fixing this?
> >
> >
> > If your hardware doesn't have any faults, you can do a `-reindex` to wipe the corrupt block file from the storage.
>
> This helped, re-indexing, but this throws away the entire progress haivng been made, and starts over at 0% :-/ it took 3+ days to get back to current state sadly w/ my hardware/network connection

Is there a way to do this from the GUI?

💬 hebasto commented on pull request "Remove the syscall sandbox":
(https://github.com/bitcoin/bitcoin/pull/27896#issuecomment-1605396572)
Concept ACK.

👍 theStack approved a pull request: "net: remove unused `CConnmanTest`"
(https://github.com/bitcoin/bitcoin/pull/27957#pullrequestreview-1496586609)
ACK 9f0d129565f1f752e771670f2df93e94f6c4d880

I was very surprised that it's even possible to specify a non-existing struct as a `friend`. Some background on this topic: https://stackoverflow.com/questions/42008500/why-is-a-friend-class-not-verified-for-existence

💬 ismaelsadeeq commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1240757480)
Is this not redundant since already checked in spend.cpp? before calling `CreateRateBumpTransaction`?

💬 ismaelsadeeq commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#issuecomment-1605463285)
tACK dff7235f9a6
Tested the `reduce_output` option on `regtest` with `bumpfee` RPC, the value of the output in the `reduced_output` was decreased to fee bump the transaction instead of adding another input.
tests fail on master and pass on dff7235f9a6.
Code looks good to me

💬 ismaelsadeeq commented on pull request "bumpfee: Allow the user to choose which output is change":
(https://github.com/bitcoin/bitcoin/pull/26467#discussion_r1240763267)
nit
```suggestion
* @param[in] outputs Vector of new outputs to replace the bumped transaction's outputs
```

💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1240773534)
Hmm ok, this should have been part of the commits ordering changes.

The change is not safe in 480f5aad, it becomes safe at the last commit (in b9fe9ab).

Basically, this last commit moves the pruning violation check from `Init()` to the end
of the 'loadblk' thread, which is where the reindex, block loading and chain activation
processes happen.

So we can run this verification step even when the reindex or reindex-chainstate flags
are enabled (which has being skipped so far).

Will
...