💬 achow101 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2535613879)
I don't think this test case is necessary, it doesn't seem to me to be testing anything that isn't already being tested.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2535613879)
I don't think this test case is necessary, it doesn't seem to me to be testing anything that isn't already being tested.
💬 achow101 commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-3543990625)
Are you still working on this? This has needed rebase for a few months now.
(https://github.com/bitcoin/bitcoin/pull/29838#issuecomment-3543990625)
Are you still working on this? This has needed rebase for a few months now.
💬 achow101 commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-3543995583)
Are you still working on this? This has needed rebase for a few months now.
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-3543995583)
Are you still working on this? This has needed rebase for a few months now.
💬 achow101 commented on pull request "psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows":
(https://github.com/bitcoin/bitcoin/pull/32419#issuecomment-3544000265)
ACK d31158d3646f3c7e4832b9ca50f6ffe02800ff4c
(https://github.com/bitcoin/bitcoin/pull/32419#issuecomment-3544000265)
ACK d31158d3646f3c7e4832b9ca50f6ffe02800ff4c
📝 theStack opened a pull request: "test: add `-alertnotify` test for large work invalid chain warning"
(https://github.com/bitcoin/bitcoin/pull/33893)
This PR adds missing test coverage for the `LARGE_WORK_INVALID_CHAIN` fork warning, checked with the `-alertnotify` option:
https://github.com/bitcoin/bitcoin/blob/ead849c9f177a3a175a22b35fa864b4b37fb9934/src/validation.cpp#L2033-L2040
Found that this is missing during review of #32587. The test works by first creating a bunch of invalid blocks, that are first announced by headers and then submitted fully in reverse (invalid tip first), in order to set `m_best_invalid` to that value, finally l
...
(https://github.com/bitcoin/bitcoin/pull/33893)
This PR adds missing test coverage for the `LARGE_WORK_INVALID_CHAIN` fork warning, checked with the `-alertnotify` option:
https://github.com/bitcoin/bitcoin/blob/ead849c9f177a3a175a22b35fa864b4b37fb9934/src/validation.cpp#L2033-L2040
Found that this is missing during review of #32587. The test works by first creating a bunch of invalid blocks, that are first announced by headers and then submitted fully in reverse (invalid tip first), in order to set `m_best_invalid` to that value, finally l
...
💬 achow101 commented on pull request "rpc, doc: clarify the response of listtransactions RPC":
(https://github.com/bitcoin/bitcoin/pull/32737#issuecomment-3544015605)
ACK c27629702285029add897b89247ab9493aede22d
(https://github.com/bitcoin/bitcoin/pull/32737#issuecomment-3544015605)
ACK c27629702285029add897b89247ab9493aede22d
📝 waketraindev opened a pull request: "net: Remove unused `local_socket_bytes` variable in `CConnman::GetAddresses()`"
(https://github.com/bitcoin/bitcoin/pull/33894)
The local_socket_bytes variable was never used. Removed it to clean up dead code.
(https://github.com/bitcoin/bitcoin/pull/33894)
The local_socket_bytes variable was never used. Removed it to clean up dead code.
🚀 achow101 merged a pull request: "psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows"
(https://github.com/bitcoin/bitcoin/pull/32419)
(https://github.com/bitcoin/bitcoin/pull/32419)
💬 achow101 commented on pull request "rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures":
(https://github.com/bitcoin/bitcoin/pull/33014#issuecomment-3544078142)
A test is definitely necessary here to avoid future regressions. @rkrux's proposed test would be fine.
The commit message has a duplicate line that needs to be removed.
(https://github.com/bitcoin/bitcoin/pull/33014#issuecomment-3544078142)
A test is definitely necessary here to avoid future regressions. @rkrux's proposed test would be fine.
The commit message has a duplicate line that needs to be removed.
🤔 mzumsande reviewed a pull request: "net: Remove unused `local_socket_bytes` variable in `CConnman::GetAddresses()`"
(https://github.com/bitcoin/bitcoin/pull/33894#pullrequestreview-3474798471)
ACK 4d893c0f46055218d6a2b3d24fbce9f0fb6ddc92
I forgot to remove this in 94db966a3bb52a3677eb5f762447202ed3889f0f (the variable was used before that).
(https://github.com/bitcoin/bitcoin/pull/33894#pullrequestreview-3474798471)
ACK 4d893c0f46055218d6a2b3d24fbce9f0fb6ddc92
I forgot to remove this in 94db966a3bb52a3677eb5f762447202ed3889f0f (the variable was used before that).
👍 theStack approved a pull request: "net: Remove unused `local_socket_bytes` variable in `CConnman::GetAddresses()`"
(https://github.com/bitcoin/bitcoin/pull/33894#pullrequestreview-3474865518)
ACK 4d893c0f46055218d6a2b3d24fbce9f0fb6ddc92
(https://github.com/bitcoin/bitcoin/pull/33894#pullrequestreview-3474865518)
ACK 4d893c0f46055218d6a2b3d24fbce9f0fb6ddc92
💬 achow101 commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3544229304)
> One comment i was about to give is "why use a .tar.gz instead of .tar, that's the only way to be sure to remove dependency on zlib", but that's a much more spread out code change.
I don't think that's true. Our guix build documentation states that the tarball needs to be extracted into `depends/SDK` (or some directory that `SDK_PATH` points to), and `depends/hosts/darwin.mk` uses the path to the extracted directory, not the tarball itself. Other than in the docs, I don't think the tarball i
...
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-3544229304)
> One comment i was about to give is "why use a .tar.gz instead of .tar, that's the only way to be sure to remove dependency on zlib", but that's a much more spread out code change.
I don't think that's true. Our guix build documentation states that the tarball needs to be extracted into `depends/SDK` (or some directory that `SDK_PATH` points to), and `depends/hosts/darwin.mk` uses the path to the extracted directory, not the tarball itself. Other than in the docs, I don't think the tarball i
...
💬 yancyribbens commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2535875154)
in 1f589e1af162c2c2705f0404496c549785f2f545 could `!state.IsValud()` be `state.IsInvalid()`?
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2535875154)
in 1f589e1af162c2c2705f0404496c549785f2f545 could `!state.IsValud()` be `state.IsInvalid()`?
⚠️ gabriellina752-max opened an issue: "Hire the Best Crypto Recovery Experts for Fast Recovery in 2025 HIRE META TECH RECOVERY PRO"
(https://github.com/bitcoin/bitcoin/issues/33895)
I just wanted to share this review with everyone who can come across it. Everyone reading this on this platform, and a great thanks to the admin who made this medium a lively one, where everyone can share their opinions. I am Mrs. Gabriel Lina, from New York, United States, and a retired nurse. I was bedridden for 8 months to get back on my feet. During this period, I came across an ad with numerous positive reviews on a trading broker platform. I was very interested, so I decided to contact the
...
(https://github.com/bitcoin/bitcoin/issues/33895)
I just wanted to share this review with everyone who can come across it. Everyone reading this on this platform, and a great thanks to the admin who made this medium a lively one, where everyone can share their opinions. I am Mrs. Gabriel Lina, from New York, United States, and a retired nurse. I was bedridden for 8 months to get back on my feet. During this period, I came across an ad with numerous positive reviews on a trading broker platform. I was very interested, so I decided to contact the
...
💬 achow101 commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3544671520)
ACK 89bbbbd257063118e6968c409e52632835b76ce8
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3544671520)
ACK 89bbbbd257063118e6968c409e52632835b76ce8
💬 achow101 commented on pull request "precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3544694241)
Seems to be a compile failure with clang:
```
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/15.2.1/../../../../include/c++/15.2.1/bits/hashtable_policy.h:1056:70: error: chosen constructor is explicit in copy-initialization
1056 | [[__no_unique_address__]] _Hashtable_ebo_helper<_Hash> _M_hash{};
| ^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/15.2.1/../../../../include/c++/15.2.1/bits/hashtable_policy.h:1062:7: no
...
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-3544694241)
Seems to be a compile failure with clang:
```
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/15.2.1/../../../../include/c++/15.2.1/bits/hashtable_policy.h:1056:70: error: chosen constructor is explicit in copy-initialization
1056 | [[__no_unique_address__]] _Hashtable_ebo_helper<_Hash> _M_hash{};
| ^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/15.2.1/../../../../include/c++/15.2.1/bits/hashtable_policy.h:1062:7: no
...
💬 achow101 commented on pull request "init: completely remove `-maxorphantx` option":
(https://github.com/bitcoin/bitcoin/pull/33872#issuecomment-3544705489)
ACK 0aebdac95da9a7d476264424c0107bd806ce5362
(https://github.com/bitcoin/bitcoin/pull/33872#issuecomment-3544705489)
ACK 0aebdac95da9a7d476264424c0107bd806ce5362
🚀 achow101 merged a pull request: "init: completely remove `-maxorphantx` option"
(https://github.com/bitcoin/bitcoin/pull/33872)
(https://github.com/bitcoin/bitcoin/pull/33872)
💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3544786316)
Friendly ping @instagibbs @glozow! in [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430443357), I think we are testing the prioritization feature so its fine if we use `invalidateblock` here. We would only want to replace `invalidateblock` during re-org testing to use fork based approach.
Why invalidateblock is OK here? It's just a test utility mechanism to quickly return a mined transaction to the mempool to verify prioritization settings persist.
Line 183 - 188:
...
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3544786316)
Friendly ping @instagibbs @glozow! in [comment](https://github.com/bitcoin/bitcoin/pull/32587#discussion_r2430443357), I think we are testing the prioritization feature so its fine if we use `invalidateblock` here. We would only want to replace `invalidateblock` during re-org testing to use fork based approach.
Why invalidateblock is OK here? It's just a test utility mechanism to quickly return a mined transaction to the mempool to verify prioritization settings persist.
Line 183 - 188:
...
💬 ajtowns commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3544844062)
> Yeah, I meant to say "less than or equal to". However, AFAICT this inequality is only enforced in standardness, not solvability.
`Solver()` calls `MatchMultisig()` which invokes `num_keys = GetScriptNuymber(opcode, data, required_sigs, MAX_PUBKEYS_PER_MULTISIG)` which returns nullopt if the CScriptNum isn't minimal and between `required_sigs` and `MAX_PUBKEYS_..`, so I believe it's enforced in both places.
> Just to be clear, we cannot allow hybrid pubkeys to be processed by `CHECKMULTISIG`
...
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3544844062)
> Yeah, I meant to say "less than or equal to". However, AFAICT this inequality is only enforced in standardness, not solvability.
`Solver()` calls `MatchMultisig()` which invokes `num_keys = GetScriptNuymber(opcode, data, required_sigs, MAX_PUBKEYS_PER_MULTISIG)` which returns nullopt if the CScriptNum isn't minimal and between `required_sigs` and `MAX_PUBKEYS_..`, so I believe it's enforced in both places.
> Just to be clear, we cannot allow hybrid pubkeys to be processed by `CHECKMULTISIG`
...