💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919484313)
> It is already explicit and will fail on master without the first commit:
>
> ```
> unknown location(0): fatal error: in "streams_tests/xor_file": std::ios_base::failure[abi:cxx11]: AutoFile::write: file handle is nullptr: iostream error
> ```

On the master branch test passes on my system (Ubuntu 22.04):
```
$ ./src/test/test_bitcoin.exe -t streams_tests/xor_file
Running 1 test case...
00f0:err:msvcrt:msvcrt_get_flags incorrect mode flag: x

*** No errors detected
```

💬 maflcko commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919490906)
> > It is already explicit and will fail on master without the first commit:
> > ```
> > unknown location(0): fatal error: in "streams_tests/xor_file": std::ios_base::failure[abi:cxx11]: AutoFile::write: file handle is nullptr: iostream error
> > ```
>
> On the master branch the test passes on my system (Ubuntu 22.04):
>
> ```
> $ ./src/test/test_bitcoin.exe -t streams_tests/xor_file
> Running 1 test case...
> 00f0:err:msvcrt:msvcrt_get_flags incorrect mode flag: x
>
> *** No erro
...

🚀 achow101 merged a pull request: "p2p: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170)

💬 epiccurious commented on pull request "log: Don't use scientific notation in "Dumped mempool" message":
(https://github.com/bitcoin/bitcoin/pull/29254#issuecomment-1919494536)
Tested ACK.


Before:
`2024-01-31T16:34:07Z Dumped mempool: 2.007e-06s to copy, 0.00121729s to dump`

With %f:
`2024-01-31T16:36:03Z Dumped mempool: 0.000002s to copy, 0.001363s to dump`

With %0.3f:
`2024-01-31T16:38:05Z Dumped mempool: 0.000s to copy, 0.001s to dump`

> I would prefer millisecond precision, but plain %.3f will output 0.000 for 0.0009, don't think that's the right thing to do, should ceil or round before output then.

A ceiling function wouldn't be right either, s
...

💬 fanquake commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919496516)
> 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.

💬 starius commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1919502670)
> > Approach NACK.
> > I get this - wasting time waiting for confirmations is more than annoying. Its effect usually lingers, because you loose context in the meantime and it slows down development in your entire organization. However, I also think supporting options that might lead to clients running disagreeing consensus rules on the same network is dangerous and might annoy developers in fresh ways. I would support the approach laid out by ajtowns in [this comment](https://github.com/bitcoin
...

💬 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.