✅ fanquake closed an issue: "Internal bug detected: "isValid == error_msg.empty()" when validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)"
(https://github.com/bitcoin/bitcoin/issues/28064)
(https://github.com/bitcoin/bitcoin/issues/28064)
💬 russeree commented on issue "Internal bug detected: "isValid == error_msg.empty()" when validateaddress(bc1p0xlxvlhemja6c4dqv22uapctqupfhlxm9h8z3k2e72q4k9hcz7v07qwwzcrf)":
(https://github.com/bitcoin/bitcoin/issues/28064#issuecomment-1630337550)
This is currently resolved in master via the addition of an else case to ```if (ConvertBits<5, 8, false>([&](unsigned char c) { data.push_back(c); }, dec.data.begin() + 1, dec.data.end()))```
here
https://github.com/bitcoin/bitcoin/blob/ef29d5d7e239b42269dd22ea94a709b5e4ceb5e5/src/key_io.cpp#L194-L197
(https://github.com/bitcoin/bitcoin/issues/28064#issuecomment-1630337550)
This is currently resolved in master via the addition of an else case to ```if (ConvertBits<5, 8, false>([&](unsigned char c) { data.push_back(c); }, dec.data.begin() + 1, dec.data.end()))```
here
https://github.com/bitcoin/bitcoin/blob/ef29d5d7e239b42269dd22ea94a709b5e4ceb5e5/src/key_io.cpp#L194-L197
💬 russeree commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259398907)
1. This should not have a period '.' at the end. Since it is inconsistent with the rest of the documentation.
2. Should start with a lowercase letter since only grammatical articles have capitalized first letters for this documentation. (A, The, This)
3. The language used in BIP22 is a more concise definition ```"longpollid" of job to monitor for expiration; required and valid only for long poll requests```
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259398907)
1. This should not have a period '.' at the end. Since it is inconsistent with the rest of the documentation.
2. Should start with a lowercase letter since only grammatical articles have capitalized first letters for this documentation. (A, The, This)
3. The language used in BIP22 is a more concise definition ```"longpollid" of job to monitor for expiration; required and valid only for long poll requests```
🤔 russeree requested changes to a pull request: "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998"
(https://github.com/bitcoin/bitcoin/pull/28056#pullrequestreview-1523750256)
Some of my thoughts about this PR. Mostly regarding the grammatical consistency of these messages.
(https://github.com/bitcoin/bitcoin/pull/28056#pullrequestreview-1523750256)
Some of my thoughts about this PR. Mostly regarding the grammatical consistency of these messages.
💬 russeree commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259399690)
Should start with a lowercase letter since only grammatical articles have capitalized first letters for this documentation. (A, The, This). Similar to review item number one.
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259399690)
Should start with a lowercase letter since only grammatical articles have capitalized first letters for this documentation. (A, The, This). Similar to review item number one.
💬 russeree commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259401850)
1. This should not have a period '.' at the end. Since it is inconsistent with the rest of the documentation.
2. Should start with a lowercase letter since only grammatical articles have capitalized first letters for this documentation. (A, The, This)
3. IMO The language used in BIP22 is a more concise definition "longpollid" of job to monitor for expiration; required and valid only for long poll requests. Others please chime in.
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259401850)
1. This should not have a period '.' at the end. Since it is inconsistent with the rest of the documentation.
2. Should start with a lowercase letter since only grammatical articles have capitalized first letters for this documentation. (A, The, This)
3. IMO The language used in BIP22 is a more concise definition "longpollid" of job to monitor for expiration; required and valid only for long poll requests. Others please chime in.
💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259409790)
Oh yes, I agree the descriptions should have the same grammatical style as the rest. I can make these changes for sure. Thanks @russeree for the help :D
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259409790)
Oh yes, I agree the descriptions should have the same grammatical style as the rest. I can make these changes for sure. Thanks @russeree for the help :D
💬 russeree commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259413067)
No problem, this is what I got running your code.
(https://github.com/bitcoin/bitcoin/pull/28056#discussion_r1259413067)
No problem, this is what I got running your code.
🚀 fanquake merged a pull request: "test: Check expected_stderr after stop"
(https://github.com/bitcoin/bitcoin/pull/28028)
(https://github.com/bitcoin/bitcoin/pull/28028)
💬 TheCharlatan commented on pull request "refactor: Move stopafterblockimport option out of blockstorage":
(https://github.com/bitcoin/bitcoin/pull/28053#issuecomment-1630478608)
Rebased 5867bb1ae7d1d345a5fd3b341d88ff777b6ef404 -> f87c21c1827fa532749ec17a2f44322378046b93 ([blockImportReturn_0](https://github.com/TheCharlatan/bitcoin/tree/blockImportReturn_0) -> [blockImportReturn_1](https://github.com/TheCharlatan/bitcoin/tree/blockImportReturn_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockImportReturn_0..blockImportReturn_1))
* Fixed conflict with #27607.
* Dropped boolean argument for making the function return early.
(https://github.com/bitcoin/bitcoin/pull/28053#issuecomment-1630478608)
Rebased 5867bb1ae7d1d345a5fd3b341d88ff777b6ef404 -> f87c21c1827fa532749ec17a2f44322378046b93 ([blockImportReturn_0](https://github.com/TheCharlatan/bitcoin/tree/blockImportReturn_0) -> [blockImportReturn_1](https://github.com/TheCharlatan/bitcoin/tree/blockImportReturn_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockImportReturn_0..blockImportReturn_1))
* Fixed conflict with #27607.
* Dropped boolean argument for making the function return early.
💬 MarcoFalke commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1630513681)
Well, it would be be good to exactly explain what this change is trying to solve and then explain why it solves the issue.
If you are trying to fix the slowness, a better fix may be to just apply `-O3` instead of `-O0` to *Bitcoin Core* and leave valgrind alone, as failing to apply optimizations is not a valgrind bug.
If you are trying to fix something else, it may be good to explain as well.
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1630513681)
Well, it would be be good to exactly explain what this change is trying to solve and then explain why it solves the issue.
If you are trying to fix the slowness, a better fix may be to just apply `-O3` instead of `-O0` to *Bitcoin Core* and leave valgrind alone, as failing to apply optimizations is not a valgrind bug.
If you are trying to fix something else, it may be good to explain as well.
💬 MarcoFalke commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1630517256)
NACK. I don't think compiling from source should be used to unintentionally and accidentally fix an unrelated bug.
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1630517256)
NACK. I don't think compiling from source should be used to unintentionally and accidentally fix an unrelated bug.
💬 MarcoFalke commented on pull request "refactor: Move stopafterblockimport option out of blockstorage":
(https://github.com/bitcoin/bitcoin/pull/28053#discussion_r1259500271)
Any reason to change this? Is there any need to remove the named constant?
(https://github.com/bitcoin/bitcoin/pull/28053#discussion_r1259500271)
Any reason to change this? Is there any need to remove the named constant?
💬 TheCharlatan commented on pull request "refactor: Move stopafterblockimport option out of blockstorage":
(https://github.com/bitcoin/bitcoin/pull/28053#issuecomment-1630541820)
Updated f87c21c1827fa532749ec17a2f44322378046b93 -> 462390c85f1174b3cf83dfb0f74922049e5cb8af ([blockImportReturn_1](https://github.com/TheCharlatan/bitcoin/tree/blockImportReturn_1) -> [blockImportReturn_2](https://github.com/TheCharlatan/bitcoin/tree/blockImportReturn_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockImportReturn_1..blockImportReturn_2))
* Addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/28053#discussion_r1259500271), re-introduced
...
(https://github.com/bitcoin/bitcoin/pull/28053#issuecomment-1630541820)
Updated f87c21c1827fa532749ec17a2f44322378046b93 -> 462390c85f1174b3cf83dfb0f74922049e5cb8af ([blockImportReturn_1](https://github.com/TheCharlatan/bitcoin/tree/blockImportReturn_1) -> [blockImportReturn_2](https://github.com/TheCharlatan/bitcoin/tree/blockImportReturn_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockImportReturn_1..blockImportReturn_2))
* Addressed @MarcoFalke's [comment](https://github.com/bitcoin/bitcoin/pull/28053#discussion_r1259500271), re-introduced
...
👍 MarcoFalke approved a pull request: "refactor: Move stopafterblockimport option out of blockstorage"
(https://github.com/bitcoin/bitcoin/pull/28053#pullrequestreview-1523927780)
lgtm ACK 462390c85f1174b3cf83dfb0f74922049e5cb8af 🗝
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 462390c85f1174b
...
(https://github.com/bitcoin/bitcoin/pull/28053#pullrequestreview-1523927780)
lgtm ACK 462390c85f1174b3cf83dfb0f74922049e5cb8af 🗝
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 462390c85f1174b
...
💬 glozow commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1259525623)
Yes, that's the idea of "reconsiderable." If you get A again with a different package, you reconsider it. If you get A+B again, you don't. In this set of changes, `TX_LOW_FEE` is used to decide to put something in the reconsiderable cache.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1259525623)
Yes, that's the idea of "reconsiderable." If you get A again with a different package, you reconsider it. If you get A+B again, you don't. In this set of changes, `TX_LOW_FEE` is used to decide to put something in the reconsiderable cache.
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1259544521)
This is tracked in https://reviews.llvm.org/D86993
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1259544521)
This is tracked in https://reviews.llvm.org/D86993
💬 MarcoFalke commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1259547933)
Can also be tested with something like on `clang++ -O0` arm64:
```cpp
#include <array>
int main() {
using A = std::array<unsigned char, 33>;
A obj{};
A copy{std::move(obj)};
obj = std::move(obj); // valid, but unspecified state
obj = std::move(copy);
}
```
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1259547933)
Can also be tested with something like on `clang++ -O0` arm64:
```cpp
#include <array>
int main() {
using A = std::array<unsigned char, 33>;
A obj{};
A copy{std::move(obj)};
obj = std::move(obj); // valid, but unspecified state
obj = std::move(copy);
}
```
💬 stickies-v commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1259566308)
I think I was confusing the kernel/node boundaries a bit. You're right that here in the kernel logic that is `validation.cpp`, the documentation and behaviour are what we'd expect.
My concern was around the _node_ implementation of `blockTip()` now potentially calling `StartShutdown()` and returning a `kernel:Interrupted` without leaving any trace. It's not something we'd expect to happen in `InvalidateBlock()` (and it would be behaviour change). I think I was wrong in suggesting we catch/log
...
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1259566308)
I think I was confusing the kernel/node boundaries a bit. You're right that here in the kernel logic that is `validation.cpp`, the documentation and behaviour are what we'd expect.
My concern was around the _node_ implementation of `blockTip()` now potentially calling `StartShutdown()` and returning a `kernel:Interrupted` without leaving any trace. It's not something we'd expect to happen in `InvalidateBlock()` (and it would be behaviour change). I think I was wrong in suggesting we catch/log
...
💬 stickies-v commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1259573661)
You make a lot of good points, and I think I agree with all of them. Thank you.
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1259573661)
You make a lot of good points, and I think I agree with all of them. Thank you.
💬 willcl-ark commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1630664257)
It's a tNACK from me currently @ c3b0f137a9 as I can no longer get this functionality to work in practice on Ubuntu 23.04 with Tor 0.4.7.13.
The send of the data in `vSocks5Init` seems to be failing, meaning the proxy cannot be used: https://github.com/pinheadmz/bitcoin/blob/c3b0f137a9ccd894f2f1d287ccdab26310b3aaf2/src/netbase.cpp#L372-L375
I haven't yet totally ruled out issues with Tor, but I do get responses from the unix socket about it not being an HTTP proxy if I try to send an HTTP
...
(https://github.com/bitcoin/bitcoin/pull/27375#issuecomment-1630664257)
It's a tNACK from me currently @ c3b0f137a9 as I can no longer get this functionality to work in practice on Ubuntu 23.04 with Tor 0.4.7.13.
The send of the data in `vSocks5Init` seems to be failing, meaning the proxy cannot be used: https://github.com/pinheadmz/bitcoin/blob/c3b0f137a9ccd894f2f1d287ccdab26310b3aaf2/src/netbase.cpp#L372-L375
I haven't yet totally ruled out issues with Tor, but I do get responses from the unix socket about it not being an HTTP proxy if I try to send an HTTP
...