💬 hebasto commented on pull request "release: Update translations for v27.0 soft translation string freeze":
(https://github.com/bitcoin/bitcoin/pull/29397#issuecomment-1933378844)
cc @fanquake
(https://github.com/bitcoin/bitcoin/pull/29397#issuecomment-1933378844)
cc @fanquake
💬 amitiuttarwar commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#issuecomment-1933460723)
ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
reviewed the code & tested the init option
(https://github.com/bitcoin/bitcoin/pull/29007#issuecomment-1933460723)
ACK 2cc8ca19f4185490f30a49516c890b2289fbab71
reviewed the code & tested the init option
💬 vasild commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1933508976)
@josibake, I see your point even though I see things a bit differently. I guess the important takeaway here is that PR is not a blocker for other stuff but a convenience of doing ser/deser in the class instead of in the caller. Thanks!
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1933508976)
@josibake, I see your point even though I see things a bit differently. I guess the important takeaway here is that PR is not a blocker for other stuff but a convenience of doing ser/deser in the class instead of in the caller. Thanks!
💬 S3RK commented on pull request "rpc: getdescriptorinfo also returns normalized descriptor":
(https://github.com/bitcoin/bitcoin/pull/29396#issuecomment-1933547285)
Concept ACK. To fix the error you need to declare new response field. (see around line no. 183)
(https://github.com/bitcoin/bitcoin/pull/29396#issuecomment-1933547285)
Concept ACK. To fix the error you need to declare new response field. (see around line no. 183)
💬 maflcko commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1933591792)
> Haven't made any progress on MSAN.
You can copy the bytes into a vector first and call `shrink_to_fit`, which should be enough to tell msan where the data really ends?
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1933591792)
> Haven't made any progress on MSAN.
You can copy the bytes into a vector first and call `shrink_to_fit`, which should be enough to tell msan where the data really ends?
💬 josibake commented on pull request "wallet: batch and simplify addressbook migration process":
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1933601910)
reACK https://github.com/bitcoin/bitcoin/commit/86960cdb7f75eaa2ae150914c54240d1d5ef96d1
Verified the diff, looks good.
(https://github.com/bitcoin/bitcoin/pull/26836#issuecomment-1933601910)
reACK https://github.com/bitcoin/bitcoin/commit/86960cdb7f75eaa2ae150914c54240d1d5ef96d1
Verified the diff, looks good.
💬 maflcko commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1933679856)
The page you link to says:
> The hardware security team identifies the developers (domain experts) who will form the initial response team for a particular issue. The initial response team can bring in further developers (domain experts) to address the issue in the best technical way.
While I am not on the security list and never was, nor have knowledge of its internals, it seems plausible to me that the recipients may pull in further developers or domain experts that are not on the list,
...
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1933679856)
The page you link to says:
> The hardware security team identifies the developers (domain experts) who will form the initial response team for a particular issue. The initial response team can bring in further developers (domain experts) to address the issue in the best technical way.
While I am not on the security list and never was, nor have knowledge of its internals, it seems plausible to me that the recipients may pull in further developers or domain experts that are not on the list,
...
💬 maflcko commented on pull request "mempool: Log added for dumping mempool transactions to disk":
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1482672568)
Up to you. It is an unrelated comment, so you can add another commit to this pull to change all in this file, or not.
(https://github.com/bitcoin/bitcoin/pull/29402#discussion_r1482672568)
Up to you. It is an unrelated comment, so you can add another commit to this pull to change all in this file, or not.
✅ maflcko closed a pull request: "test: handle wallet_reorgrestore.py failed"
(https://github.com/bitcoin/bitcoin/pull/29395)
(https://github.com/bitcoin/bitcoin/pull/29395)
💬 maflcko commented on pull request "test: handle wallet_reorgrestore.py failed":
(https://github.com/bitcoin/bitcoin/pull/29395#issuecomment-1933738018)
I am not sure if your replies are ChatGPT genereated or not, but this clearly isn't the correct fix.
If your replies are ChatGPT generated, please don't. If anyone was interested in computer-generated hallucinations, they could do so themselves.
It is not the correct fix, because:
* The race isn't explained, either theoretically by pointing to the exact line(s) in the source code where it happens, or practically by adding a sleep to force the bug to appear for testing, as explained befo
...
(https://github.com/bitcoin/bitcoin/pull/29395#issuecomment-1933738018)
I am not sure if your replies are ChatGPT genereated or not, but this clearly isn't the correct fix.
If your replies are ChatGPT generated, please don't. If anyone was interested in computer-generated hallucinations, they could do so themselves.
It is not the correct fix, because:
* The race isn't explained, either theoretically by pointing to the exact line(s) in the source code where it happens, or practically by adding a sleep to force the bug to appear for testing, as explained befo
...
✅ maflcko closed an issue: "Bitcoin ubuntu ppa, outdated"
(https://github.com/bitcoin/bitcoin/issues/29406)
(https://github.com/bitcoin/bitcoin/issues/29406)
💬 maflcko commented on issue "Bitcoin ubuntu ppa, outdated":
(https://github.com/bitcoin/bitcoin/issues/29406#issuecomment-1933755423)
The PPA was retired, because it didn't use the deterministically generated guix release builds. However, it seems unlikely to be maintained again, unless a new maintainer steps up to maintain it going forward.
In any case, there are endless other ways to get the latest version of Bitcoin Core:
* Self-compile any release tag. Ref: https://github.com/bitcoin/bitcoin/releases
* Download the deterministic release binary. Ref: https://bitcoincore.org/en/download/
* Use the snap, flat, guix or
...
(https://github.com/bitcoin/bitcoin/issues/29406#issuecomment-1933755423)
The PPA was retired, because it didn't use the deterministically generated guix release builds. However, it seems unlikely to be maintained again, unless a new maintainer steps up to maintain it going forward.
In any case, there are endless other ways to get the latest version of Bitcoin Core:
* Self-compile any release tag. Ref: https://github.com/bitcoin/bitcoin/releases
* Download the deterministic release binary. Ref: https://bitcoincore.org/en/download/
* Use the snap, flat, guix or
...
💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1482735029)
Ah right, the nodes are on different chains, so the utxo sets the miniwallet can use are different.
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1482735029)
Ah right, the nodes are on different chains, so the utxo sets the miniwallet can use are different.
💬 maflcko commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#discussion_r1482747927)
Ah, will undo if I retouch.
(https://github.com/bitcoin/bitcoin/pull/29114#discussion_r1482747927)
Ah, will undo if I retouch.
💬 maflcko commented on pull request "util: Faster std::byte (pre)vector (un)serialize":
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1933790588)
> It's a nice cleanup, but I don't observe a big speedup on my machine after applying the diff and running the bench mentioned in the description on top of [fab4169](https://github.com/bitcoin/bitcoin/commit/fab41697a5448ef2861f65795bd63a4ccdda6a40).
Can you clarify if there is no speedup, or just a smaller one? Also, what it the speed for `uint8_t` on master vs `std::byte` on master vs `std::byte` after this pull?
(https://github.com/bitcoin/bitcoin/pull/29114#issuecomment-1933790588)
> It's a nice cleanup, but I don't observe a big speedup on my machine after applying the diff and running the bench mentioned in the description on top of [fab4169](https://github.com/bitcoin/bitcoin/commit/fab41697a5448ef2861f65795bd63a4ccdda6a40).
Can you clarify if there is no speedup, or just a smaller one? Also, what it the speed for `uint8_t` on master vs `std::byte` on master vs `std::byte` after this pull?
💬 TheCharlatan commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1933987554)
I validated the patch by running:
\> `iwyu_tool -p . -j $(nproc) -- -Xiwyu --max_line_length=180 -I/usr/lib/llvm-14/lib/clang/14/include/ -Xiwyu --mapping_file=/home/drgrid/bitcoin/contrib/devtools/iwyu/bitcoin.core.imp > iwyu_output.txt`
\> `cat iwyu_output.txt | awk '/should remove these lines:/ {file=$1} /- #include <config\/bitcoin-config.h>/ {split($NF, a, "-"); line=a[1]; if (!seen[file, line]++) print file " " a[1]}' | while read -r file line; do sed -i -e "$((line-1)),$((line+2))d" "sr
...
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1933987554)
I validated the patch by running:
\> `iwyu_tool -p . -j $(nproc) -- -Xiwyu --max_line_length=180 -I/usr/lib/llvm-14/lib/clang/14/include/ -Xiwyu --mapping_file=/home/drgrid/bitcoin/contrib/devtools/iwyu/bitcoin.core.imp > iwyu_output.txt`
\> `cat iwyu_output.txt | awk '/should remove these lines:/ {file=$1} /- #include <config\/bitcoin-config.h>/ {split($NF, a, "-"); line=a[1]; if (!seen[file, line]++) print file " " a[1]}' | while read -r file line; do sed -i -e "$((line-1)),$((line+2))d" "sr
...
💬 TheCharlatan commented on pull request "RFC: build: remove confusing and inconsistent disable-asm option":
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1933988455)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29407#issuecomment-1933988455)
Concept ACK
💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1482919503)
Yes, I'll address it.
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1482919503)
Yes, I'll address it.
💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1482929854)
Where is it calling `NetPermissions::Initialize` again?
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1482929854)
Where is it calling `NetPermissions::Initialize` again?
💬 MarnixCroes commented on pull request "debugwindow: update session ID tooltip":
(https://github.com/bitcoin-core/gui/pull/788#issuecomment-1934067523)
> > When you have a v2 connection, there is always a session ID.
>
> What if the connection type is still `DETECTING`?
>
> The code reference:
>
> https://github.com/bitcoin-core/gui/blob/6737331c4ccc6da578bb44c809cc4e18f017ba51/src/node/connection_types.h#L83-L88
then there is no session ID property
as it is not a v2 peer (yet), right
(https://github.com/bitcoin-core/gui/pull/788#issuecomment-1934067523)
> > When you have a v2 connection, there is always a session ID.
>
> What if the connection type is still `DETECTING`?
>
> The code reference:
>
> https://github.com/bitcoin-core/gui/blob/6737331c4ccc6da578bb44c809cc4e18f017ba51/src/node/connection_types.h#L83-L88
then there is no session ID property
as it is not a v2 peer (yet), right