💬 fanquake commented on pull request "refactor: Remove redundant checks in compat/assumptions.h":
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814186095)
Concept ACK. We need to fixup/remove the __cplusplus so we can do C++20 while also supporting GCC 10.
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814186095)
Concept ACK. We need to fixup/remove the __cplusplus so we can do C++20 while also supporting GCC 10.
💬 maflcko commented on pull request "refactor: Remove redundant checks in compat/assumptions.h":
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814190598)
> Concept ACK. We need to fixup/remove the __cplusplus so we can do C++20 while also supporting GCC 10.
Yeah, it would be a bit cleaner style-wise, if this was merged before C++20. However, it is not a blocker, because the check accepts anything greater than C++17, which should always pass for all supported compilers:
```
static_assert(__cplusplus >= 201703L, "C++17 standard assumed");
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814190598)
> Concept ACK. We need to fixup/remove the __cplusplus so we can do C++20 while also supporting GCC 10.
Yeah, it would be a bit cleaner style-wise, if this was merged before C++20. However, it is not a blocker, because the check accepts anything greater than C++17, which should always pass for all supported compilers:
```
static_assert(__cplusplus >= 201703L, "C++17 standard assumed");
💬 maflcko commented on pull request "refactor: Remove redundant checks in compat/assumptions.h":
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814192749)
I was mostly waiting for a re-reply from @theuni and @hebasto. Though, given that they haven't replied in more than a month now, I presume they are fine with this pull in the current form?
(https://github.com/bitcoin/bitcoin/pull/28579#issuecomment-1814192749)
I was mostly waiting for a re-reply from @theuni and @hebasto. Though, given that they haven't replied in more than a month now, I presume they are fine with this pull in the current form?
💬 fanquake commented on pull request "tests: Fix LCOV_OPTS to be in the correct position":
(https://github.com/bitcoin/bitcoin/pull/28771#issuecomment-1814195738)
I think `--rc` is different case, not included in that list, so it is also still working here?
> --rc keyword=value
> Override a configuration directive.
> Use this option to specify a keyword=value statement which overrides the corresponding configuration statement in the > lcovrc configuration file. You can specify this option more than once to override multiple configuration statements. See > [lcovrc(5)](https://man.archlinux.org/man/lcovrc.5.en) for a list of available keywords and th
...
(https://github.com/bitcoin/bitcoin/pull/28771#issuecomment-1814195738)
I think `--rc` is different case, not included in that list, so it is also still working here?
> --rc keyword=value
> Override a configuration directive.
> Use this option to specify a keyword=value statement which overrides the corresponding configuration statement in the > lcovrc configuration file. You can specify this option more than once to override multiple configuration statements. See > [lcovrc(5)](https://man.archlinux.org/man/lcovrc.5.en) for a list of available keywords and th
...
💬 fanquake commented on pull request "rpc: Remove deprecated -rpcserialversion":
(https://github.com/bitcoin/bitcoin/pull/28890#issuecomment-1814199845)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/28890#issuecomment-1814199845)
Concept ACK.
🤔 BrandonOdiwuor reviewed a pull request: "test: Use test framework utils in functional tests"
(https://github.com/bitcoin/bitcoin/pull/28528#pullrequestreview-1734132495)
There are still some instances of bare assert which could be replaced by the helpers such as in `rpc_psbt.py`, `wallet_disable.py` is this intentional?
(https://github.com/bitcoin/bitcoin/pull/28528#pullrequestreview-1734132495)
There are still some instances of bare assert which could be replaced by the helpers such as in `rpc_psbt.py`, `wallet_disable.py` is this intentional?
💬 willcl-ark commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395555866)
Agree that it should be `true` as a belt-and-suspenders, but we are inside `if (CheckMinimalPush(vch, opcode))` already here, so shouldn't happen?
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395555866)
Agree that it should be `true` as a belt-and-suspenders, but we are inside `if (CheckMinimalPush(vch, opcode))` already here, so shouldn't happen?
📝 theStack opened a pull request: "test: fix `AddNode` unit test failure on OpenBSD"
(https://github.com/bitcoin/bitcoin/pull/28891)
On OpenBSD 7.4, the following check of the unit test `test_addnode_getaddednodeinfo_and_connection_detection` currently fails:
```
BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.1", /*m_use_v2transport=*/true}));
```
The reason for that is that this OS seemingly doesn't support the IPv4 shorthand notation with omitted zero-bytes:
```
$ ping 127.1
ping: no address associated with name
```
As a simple fix, this PR skips the check for this with a pre-processor #if. On NetBSD and
...
(https://github.com/bitcoin/bitcoin/pull/28891)
On OpenBSD 7.4, the following check of the unit test `test_addnode_getaddednodeinfo_and_connection_detection` currently fails:
```
BOOST_CHECK(!connman->AddNode({/*m_added_node=*/"127.1", /*m_use_v2transport=*/true}));
```
The reason for that is that this OS seemingly doesn't support the IPv4 shorthand notation with omitted zero-bytes:
```
$ ping 127.1
ping: no address associated with name
```
As a simple fix, this PR skips the check for this with a pre-processor #if. On NetBSD and
...
💬 theStack commented on pull request "test: fix `AddNode` unit test failure on OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/28891#issuecomment-1814286184)
cc @pinheadmz who is apparently also running an OpenBSD server, as far as I remember :-)
(https://github.com/bitcoin/bitcoin/pull/28891#issuecomment-1814286184)
cc @pinheadmz who is apparently also running an OpenBSD server, as far as I remember :-)
✅ maflcko closed a pull request: "Sanitizing ports of -rpcconnect and -rpcport."
(https://github.com/bitcoin/bitcoin/pull/27820)
(https://github.com/bitcoin/bitcoin/pull/27820)
💬 ajtowns commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395576175)
Byte code `024000` is a minimal push of the two-byte vector `4000` (so passes `CheckMinimalPush`) but it's not a minimal `CScriptNum` since it just evaluates to 64, and should have been `0140` (so fails `fRequireMinimal` due to the trailing 0).
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395576175)
Byte code `024000` is a minimal push of the two-byte vector `4000` (so passes `CheckMinimalPush`) but it's not a minimal `CScriptNum` since it just evaluates to 64, and should have been `0140` (so fails `fRequireMinimal` due to the trailing 0).
✅ maflcko closed a pull request: "CLI: Only one Request Handler can be specified."
(https://github.com/bitcoin/bitcoin/pull/27815)
(https://github.com/bitcoin/bitcoin/pull/27815)
✅ maflcko closed a pull request: "Blocking arguments -nohelp, -noh, and -no?"
(https://github.com/bitcoin/bitcoin/pull/27814)
(https://github.com/bitcoin/bitcoin/pull/27814)
💬 maflcko commented on pull request "Blocking arguments -nohelp, -noh, and -no?":
(https://github.com/bitcoin/bitcoin/pull/27814#issuecomment-1814292353)
Closing for now, due to lack of activity. Let us know if you want to pick this up again, and if this should be reopened.
(https://github.com/bitcoin/bitcoin/pull/27814#issuecomment-1814292353)
Closing for now, due to lack of activity. Let us know if you want to pick this up again, and if this should be reopened.
💬 ajtowns commented on pull request "rpc: Remove deprecated -rpcserialversion":
(https://github.com/bitcoin/bitcoin/pull/28890#issuecomment-1814295998)
> It is deprecated
It's only been deprecated in master for a couple of months; better to wait for it to actually be deprecated in the current release for a little while to give people a chance to complain if they're still relying on this functionality?
Otherwise, changes here look like what I'd expect.
> If there is a use-case, likely a per-RPC flag can be added, if needed.
Could give bitcoin-util the capability to strip witness data from a block/tx and leave it up to users who need
...
(https://github.com/bitcoin/bitcoin/pull/28890#issuecomment-1814295998)
> It is deprecated
It's only been deprecated in master for a couple of months; better to wait for it to actually be deprecated in the current release for a little while to give people a chance to complain if they're still relying on this functionality?
Otherwise, changes here look like what I'd expect.
> If there is a use-case, likely a per-RPC flag can be added, if needed.
Could give bitcoin-util the capability to strip witness data from a block/tx and leave it up to users who need
...
💬 maflcko commented on pull request "rpc: Remove deprecated -rpcserialversion":
(https://github.com/bitcoin/bitcoin/pull/28890#issuecomment-1814309383)
> better to wait for it to actually be deprecated in the current release for a little while to give people a chance to complain if they're still relying on this functionality?
Happy to wait, but there shouldn't be any merge conflicts, so it should be trivial to just type `git revert` cleanly in the rare case that the deprecation will be kicked down the line one more release or so.
(https://github.com/bitcoin/bitcoin/pull/28890#issuecomment-1814309383)
> better to wait for it to actually be deprecated in the current release for a little while to give people a chance to complain if they're still relying on this functionality?
Happy to wait, but there shouldn't be any merge conflicts, so it should be trivial to just type `git revert` cleanly in the rare case that the deprecation will be kicked down the line one more release or so.
💬 hebasto commented on issue "build: Configuring with `-mno-sse4.1` does not fail the sse4.1 instrinsics check":
(https://github.com/bitcoin/bitcoin/issues/28864#issuecomment-1814316786)
FWIW, adding `-O1` to the test makes it fail with `CXXFLAGS="-mno-sse4.1"`.
(https://github.com/bitcoin/bitcoin/issues/28864#issuecomment-1814316786)
FWIW, adding `-O1` to the test makes it fail with `CXXFLAGS="-mno-sse4.1"`.
👋 TheCharlatan's pull request is ready for review: "refactor: Replace sets of txiter with CTxMemPoolEntryRefs"
(https://github.com/bitcoin/bitcoin/pull/28886)
(https://github.com/bitcoin/bitcoin/pull/28886)
💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395623337)
Yeah, there are two distinct concepts of minimality:
* Minimal pushes apply to the interpretation of stack elements as *byte vectors*, and are about whether you used the right opcode (OP_n, direct push, OP_PUSHDATA1, OP_PUSHDATA2, OP_PUSHDATA4): you have to use the first applicable one from that list for a push to be a minimal push.
* Minimally-encoded integers apply to the interpretation of stack elements as *numbers*, and are about whether no surplus bytes we introduced when encoding the num
...
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395623337)
Yeah, there are two distinct concepts of minimality:
* Minimal pushes apply to the interpretation of stack elements as *byte vectors*, and are about whether you used the right opcode (OP_n, direct push, OP_PUSHDATA1, OP_PUSHDATA2, OP_PUSHDATA4): you have to use the first applicable one from that list for a push to be a minimal push.
* Minimally-encoded integers apply to the interpretation of stack elements as *numbers*, and are about whether no surplus bytes we introduced when encoding the num
...
💬 TheCharlatan commented on pull request "RFC: Remove boost usage from kernel api / headers":
(https://github.com/bitcoin/bitcoin/pull/28335#discussion_r1395656090)
Added a new commit for this.
(https://github.com/bitcoin/bitcoin/pull/28335#discussion_r1395656090)
Added a new commit for this.