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

💬 fanquake commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919469009)
Concept NACK. I don't think we should remove test coverage from all other platforms, just to accomodate Windows. In the worse case this should just have a Windows paths with the modified modifiers, with documentation explaining why?

💬 furszy commented on pull request "rpc: Drop migratewallet experimental warning":
(https://github.com/bitcoin/bitcoin/pull/28037#issuecomment-1919470797)
Also, we do have other bug fixes that should be included in v27 to deprecate this warning.
See #28976, #28868, #29112. Would be nice to bring more attention to them.

💬 maflcko commented on pull request "wallet: simplify and batch zap wallet txes process":
(https://github.com/bitcoin/bitcoin/pull/28987#discussion_r1473125056)
nit: Any reason to add `__func__` to this log, when it previously wasn't there? If someone is interested in the exact source location, they can enable the corresponding log option. If this is needed for context, it could make more sense to clarify the log message itself, so that it is unique and clear without requiring the source code as context.

👍 stickies-v approved a pull request: "refactor: Fix prevector iterator concept issues"
(https://github.com/bitcoin/bitcoin/pull/29275#pullrequestreview-1854275203)
ACK fad74bbbd0ab4425573f182ebde1b31a99e80547

💬 hebasto commented on pull request "test: Drop `x` modifier in `fsbridge::fopen` call":
(https://github.com/bitcoin/bitcoin/pull/29357#issuecomment-1919481274)
> Concept NACK. I don't think we should remove test coverage from all other platforms, just to accomodate Windows.

It is not about Windows as a platform. It is about Windows users who download the Bitcoin Core release binaries. We should not treat them in a discriminatory way.

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