💬 maflcko commented on pull request "log: Don't use scientific notation in "Dumped mempool" message":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1919503489)
Are you still working on this, or should it be marked "up for grabs"?

💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919510092)
> Can you add exact steps to reproduce?
```
$ cat /etc/os-release | head -1
PRETTY_NAME="Ubuntu 22.04.3 LTS"
$ x86_64-w64-mingw32-g++-posix --version
x86_64-w64-mingw32-g++-posix (GCC) 10-posix 20220113
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ make -C depends -j $(nproc) HOST=x86_64-w64-mingw32 NO_QT=1
$ ./autogen.sh && ./co
...

👍 ryanofsky approved a pull request: "wallet: guard against dangling to-be-reverted db transactions"
(https://github.com/bitcoin/bitcoin/pull/29253#pullrequestreview-1854204356)
Code review ACK b298242c8d495c36072415e1b95eaa7bf485a38a. Just fix for exec result codes and comment update since last review.

Overall this change should help wallet recover and not cause more problems if a rollback fails, and it adds some nice test coverage.

💬 ryanofsky commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1473087294)
In commit "sqlite: add ability to interrupt statements" (dca874e838c2599bd24433675b291168f8e7b055)

It's a little better to pass plain `unique_ptr` instead of `unique_ptr&&`, because former guarantees unique_ptr argument will be moved-from and null afterward, while latter means the move may or may not happen depending on internals of the function. In general you want explicit && references for template programming and perfect forwarding, but should use values and & references otherwise.. Can s
...

💬 epiccurious commented on pull request "log: Don't use scientific notation in "Dumped mempool" message":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1919542546)
@kristapsk if you prefer, I'd be happy to see it through.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1473174956)
> I'm not sure if this should be optional. Was thinking before about asserting here. Wdyt?

I don't see a benefit to aborting when the wait_callback option is true and there is no callback. There is no callback if the transaction is already in the mempool, and we are ok with ignoring the wait_callback option in that case. Would suggest changing line 95 to `if (wait_callback && node.validation_signals)`

💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919554426)
> > We should not treat them in a discriminatory way.
>
> I don't see how fixing the tests in the way I've suggested would be discriminatory. It would make them work on Windows while retaining the current test for all other platforms.

Thank you! Your suggestion has been implemented.

💬 stratospher commented on pull request "test: fix intermittent failure in p2p_v2_earlykeyresponse":
(https://github.com/bitcoin/bitcoin/pull/29352#issuecomment-1919569103)
tested ACK 9642aef.

> Sounds reasonable to me, but I'd like to refer that to @stratospher, who I believe has plans to extend / rewrite the test anyway (see the WIP branch linked https://github.com/bitcoin/bitcoin/pull/24748#issuecomment-1917230742), and just fix the CI here straightforwardly.

yes, will look into it! extending this test for more v2 disconnection scenarios in [this WIP branch](https://github.com/stratospher/bitcoin/blob/6bc26fb6e1803481e773f647ddbe299fd2977877/test/functiona
...

💬 maflcko commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1919572281)
Rebase for fresh CI?

💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1919591341)
Thank you for the review @furszy!

Updated 536429372dfd9759dc8f089962f3a15a94b54751 -> cb47c334e0b3d654b5b56ae762aa2a2c33ae2743 ([noGlobalSignals_11](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_11) -> [noGlobalSignals_12](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_12), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_11..noGlobalSignals_12))

* Addressed @furszy's [comment](https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1
...

💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1919611531)
Concept ACK

My scenario for using both `maxtxfee` and `maxfeerate` would be slightly different. For example looking at the mempool in January, I might generally be okay with paying up to 50 sat/vB, but not a higher feerate, because I expect that it will generally suffice to get a confirmation within 24h, but at such a high feerate, I would not want to make a transaction with more than 10 inputs. So, I’d delimit the feerate itself on the one hand, and then also limit the `maxtxfee` to 35'000 s
...

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919617791)
> > Can you add exact steps to reproduce?
>
> ```
> $ cat /etc/os-release | head -1
> PRETTY_NAME="Ubuntu 22.04.3 LTS"
> $ x86_64-w64-mingw32-g++-posix --version
> x86_64-w64-mingw32-g++-posix (GCC) 10-posix 20220113
> Copyright (C) 2020 Free Software Foundation, Inc.
> This is free software; see the source for copying conditions. There is NO
> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
>
> $ make -C depends -j $(nproc) HOST=x86_64-w64-mingw32 NO_QT
...

💬 theStack commented on pull request "test: make v2transport arg in addconnection mandatory and few cleanups":
(https://github.com/bitcoin/bitcoin/pull/29356#issuecomment-1919629659)
Concept ACK

Another possible alternative for `addconnection`'s third parameter handling would be to take as default whatever is set by `-v2transport`, like we already do it for `addnode` (i.e. `bool use_v2transport = self.MaybeArg<bool>(2).value_or(node_v2transport);`).

💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919640104)
> I also tried this locally, and the file is not a nullptr.

I agree. That is how it works on Linux in the Wine environment.

On Windows, being linked to `msvcrt.dll`, it fails.

💬 josibake commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473270005)
Summarizing my thoughts from some offline discussion with @ismaelsadeeq :

* I'm fairly certain we don't check `relayMinFee` when creating a transaction, but it would be good to confirm this (even better, to have a functional test!)
* If the user can set both `relayMinFee` and `maxtxfee`, I think we need to have a sanity check at startup to make sure they are not setting insane values that could result in them creating stuck transactions
* I think its okay to use a "fake transaction size" fo
...

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call for MinGW builds":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919669199)
> > I also tried this locally, and the file is not a nullptr.
>
> I agree. That is how it works on Linux in the Wine environment.
>
> On Windows, being linked to `msvcrt.dll`, it fails.

Yes, either the file is a nullptr, in which case the test already fails, or it is not, in which case the test passes. In any case, the added assert in the dropped commit isn't needed here or elsewhere, and doesn't change the outcome here.

🤔 murchandamus reviewed a pull request: "Wallet: Add `maxfeerate` wallet startup option"
(https://github.com/bitcoin/bitcoin/pull/29278#pullrequestreview-1854463255)
Concept ACK

💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473230950)
We are comparing two feerates, should this perhaps be:

```suggestion
throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("fee rate cannot be more than wallet max tx fee rate (%s)", pwallet->m_max_tx_fee_rate.ToString()));
```

💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473259190)
In a4b93767df8ce3f2b688b5295066965f4854f40f (wallet: test: enforce -maxfeerate on wallet transactions):

I am a bit confused. You marked this as resolved and said you added this, but I don’t see it in the current version. Did you fix it in your working branch but haven’t pushed yet?

💬 murchandamus commented on pull request "Wallet: Add `maxfeerate` wallet startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#discussion_r1473237266)
In 7664ec9bd744ab5ae403a87cd9d2aa3de747d278 (wallet: add `maxfeerate` wallet startup option):

`walletInstance->m_default_max_tx_fee` indicates to me that the value is the _default_ if the user does not customize the value, so it is confusing to me that the user value is assigned to a variable with "default" in its name.

```suggestion
walletInstance->m_max_tx_fee = max_fee.value();
```