💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2071116761)
> No, it isn't. Nor is piling on the `EVENT_CFLAGS`.
Removed both, thanks @hebasto and @laanwj !
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2071116761)
> No, it isn't. Nor is piling on the `EVENT_CFLAGS`.
Removed both, thanks @hebasto and @laanwj !
💬 jrakibi commented on pull request "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply":
(https://github.com/bitcoin/bitcoin/pull/29617#discussion_r1575459383)
Fixed and rebased.
(https://github.com/bitcoin/bitcoin/pull/29617#discussion_r1575459383)
Fixed and rebased.
💬 asctime commented on issue "ReadAnchor throws exception on second run":
(https://github.com/bitcoin/bitcoin/issues/29931#issuecomment-2071117960)
No I cannot reproduce with the binary distro, at least on Linux. Different compilers seem to manage the exception slightly differently. Regardless I'm not sure that a blind delete is the right way to do this, is all. Fixed on my side, I just wanted to point it out.
(https://github.com/bitcoin/bitcoin/issues/29931#issuecomment-2071117960)
No I cannot reproduce with the binary distro, at least on Linux. Different compilers seem to manage the exception slightly differently. Regardless I'm not sure that a blind delete is the right way to do this, is all. Fixed on my side, I just wanted to point it out.
💬 fjahr commented on pull request "test: Validate UTXO snapshot with coin height > base height & amount > MAX_MONEY supply":
(https://github.com/bitcoin/bitcoin/pull/29617#issuecomment-2071123104)
re-ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a47
(https://github.com/bitcoin/bitcoin/pull/29617#issuecomment-2071123104)
re-ACK ec1f1abfefa281e62bb876aa1c4738d576ef9a47
💬 tdb3 commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1575519821)
Thanks for taking a look and trying it out @achow101. Much appreciated.
Good point. Yes, the test would fail if an independent node is running on 18443.
Although not ideal, do you think this would be reasonable since a warning is provided when bitcoind is running independently? (i.e. `WARNING! There is already a bitcoind process running on this system. Tests may fail unexpectedly due to resource contention!`)
Alternatively, the assert statement could be removed, but that seems a bit lac
...
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1575519821)
Thanks for taking a look and trying it out @achow101. Much appreciated.
Good point. Yes, the test would fail if an independent node is running on 18443.
Although not ideal, do you think this would be reasonable since a warning is provided when bitcoind is running independently? (i.e. `WARNING! There is already a bitcoind process running on this system. Tests may fail unexpectedly due to resource contention!`)
Alternatively, the assert statement could be removed, but that seems a bit lac
...
💬 tdb3 commented on pull request "test: Run framework unit tests in parallel":
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2071244090)
> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).
Rebased to address conflict
(https://github.com/bitcoin/bitcoin/pull/29771#issuecomment-2071244090)
> 🐙 This pull request conflicts with the target branch and [needs rebase](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes).
Rebased to address conflict
💬 maflcko commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1575636802)
I'd say it should be avoided to use test outside of the allowed range assigned to this test case. Each test is assigned 12, or so, p2p and rpc ports. I think this test can be dropped, because if the default behavior changed, pretty much every developer would notice quickly anyway?
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1575636802)
I'd say it should be avoided to use test outside of the allowed range assigned to this test case. Each test is assigned 12, or so, p2p and rpc ports. I think this test can be dropped, because if the default behavior changed, pretty much every developer would notice quickly anyway?
💬 maflcko commented on issue "ReadAnchor throws exception on second run":
(https://github.com/bitcoin/bitcoin/issues/29931#issuecomment-2071409336)
> Different compilers seem to manage the exception slightly differently?
That should not be the case. The point of standard C++ library `std::filesystem::remove` is to provide the same interface behavior, regardless of compiler or operating system.
The specification says that `false` should be returned when the file does not exist, not an exception be thrown.
However, without exact steps to reproduce, using your compiler version, there is little that can be done here.
> Regardless I'
...
(https://github.com/bitcoin/bitcoin/issues/29931#issuecomment-2071409336)
> Different compilers seem to manage the exception slightly differently?
That should not be the case. The point of standard C++ library `std::filesystem::remove` is to provide the same interface behavior, regardless of compiler or operating system.
The specification says that `false` should be returned when the file does not exist, not an exception be thrown.
However, without exact steps to reproduce, using your compiler version, there is little that can be done here.
> Regardless I'
...
💬 maflcko commented on issue "ReadAnchor throws exception on second run":
(https://github.com/bitcoin/bitcoin/issues/29931#issuecomment-2071410653)
According to https://github.com/bitcoin/bitcoin/issues/29930#issuecomment-2071114099 the reason could be that you are using an experimental C++17 standard library, which has bugs?
(https://github.com/bitcoin/bitcoin/issues/29931#issuecomment-2071410653)
According to https://github.com/bitcoin/bitcoin/issues/29930#issuecomment-2071114099 the reason could be that you are using an experimental C++17 standard library, which has bugs?
💬 maflcko commented on pull request "[DO NOT MERGE] testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2071451850)
> I doubt many people do that.
Two people raised the concern in this thread, so why would you doubt it?
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2071451850)
> I doubt many people do that.
Two people raised the concern in this thread, so why would you doubt it?
💬 maflcko commented on pull request "doc: add LLVM instruction for macOS < 13":
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575669167)
Sure, makes sense. It is just a nit, and I guess the text will be removed after a bump of `OSX_MIN_VERSION` anyway.
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575669167)
Sure, makes sense. It is just a nit, and I guess the text will be removed after a bump of `OSX_MIN_VERSION` anyway.
💬 maflcko commented on issue "RFC: In guix compile the GUI sequentially from everything else?":
(https://github.com/bitcoin/bitcoin/issues/29914#issuecomment-2071458074)
> FWIW in https://github.com/bitcoin/bitcoin/pull/29923 i've removed all the GUI specific build-time dependencies except for Qt itself.
Nice!
`qt` is still a massive blob (and possibly a large attack surface for backdoors), so I guess sequential builds could still be considered, but the priority would be less urgent after 29923.
(https://github.com/bitcoin/bitcoin/issues/29914#issuecomment-2071458074)
> FWIW in https://github.com/bitcoin/bitcoin/pull/29923 i've removed all the GUI specific build-time dependencies except for Qt itself.
Nice!
`qt` is still a massive blob (and possibly a large attack surface for backdoors), so I guess sequential builds could still be considered, but the priority would be less urgent after 29923.
✅ maflcko closed an issue: "Intermittent issue in p2p_handshake.py", line 75, in run_test self.test_desirable_service_flags(node, [NODE_NETWORK | NODE_WITNESS], not true after 2400.0 seconds"
(https://github.com/bitcoin/bitcoin/issues/29896)
(https://github.com/bitcoin/bitcoin/issues/29896)
💬 fanquake commented on pull request "doc: add LLVM instruction for macOS < 13":
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575676917)
> That's not that long ago though, and we have `OSX_MIN_VERSION=11.0` for now.
Just because we build release binaries that support that operating system, does not mean you need to be able to compile on that system (regardless of using the system compiler).
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575676917)
> That's not that long ago though, and we have `OSX_MIN_VERSION=11.0` for now.
Just because we build release binaries that support that operating system, does not mean you need to be able to compile on that system (regardless of using the system compiler).
💬 fanquake commented on pull request "doc: add LLVM instruction for macOS < 13":
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575678033)
Why would LLVM not work? We use the near latest release of LLVM to build the macOS release binaries. There's no point making this LLVM 14 just to "bump it" almost immediately. We should just point to the latest version unless there's an actual reason not to.
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575678033)
Why would LLVM not work? We use the near latest release of LLVM to build the macOS release binaries. There's no point making this LLVM 14 just to "bump it" almost immediately. We should just point to the latest version unless there's an actual reason not to.
💬 knocte commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2071479147)
cACK
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2071479147)
cACK
💬 maflcko commented on pull request "contrib: rpcauth.py - Add new option (-json) to output text in json format":
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-2071512668)
Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/29433#issuecomment-2071512668)
Are you still working on this?
💬 maflcko commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-2071541769)
Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-2071541769)
Are you still working on this?
💬 Sjors commented on pull request "doc: add LLVM instruction for macOS < 13":
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575734223)
That's also true, though so far I haven't seen evidence that compilation is impossible.
Homebrews main motivation for dropping macOS 11 seems to be support requests. From their install script:
> This installation may not succeed.
After installation, you will encounter build failures with some formulae.
Please create pull requests instead of asking for help on Homebrew\'s GitHub,
Twitter or any other official channels. You are responsible for resolving any
issues you experience while yo
...
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575734223)
That's also true, though so far I haven't seen evidence that compilation is impossible.
Homebrews main motivation for dropping macOS 11 seems to be support requests. From their install script:
> This installation may not succeed.
After installation, you will encounter build failures with some formulae.
Please create pull requests instead of asking for help on Homebrew\'s GitHub,
Twitter or any other official channels. You are responsible for resolving any
issues you experience while yo
...
💬 maflcko commented on pull request "doc: add LLVM instruction for macOS < 13":
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575739845)
I guess someone will need to test either way. Once they have tested with `brew install llvm` vs `brew install llvm@_version_`, this can be bumped to the maximum working `_version_` (or none at all).
(https://github.com/bitcoin/bitcoin/pull/29934#discussion_r1575739845)
I guess someone will need to test either way. Once they have tested with `brew install llvm` vs `brew install llvm@_version_`, this can be bumped to the maximum working `_version_` (or none at all).