💬 hebasto commented on issue "Apple Clang 14.0 lacks support for `std::is_eq`":
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2067717721)
> Faced this too. Updating to 14.0.3 fixes the problem.
Well, that's actually means switching to the toolchain based on LLVM 15.
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2067717721)
> Faced this too. Updating to 14.0.3 fixes the problem.
Well, that's actually means switching to the toolchain based on LLVM 15.
💬 Gary19751957 commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2067721400)
Why am I receiving these emails?This person is using my email to hack my
Bitcoin account,please help, Gary Rollins my phone number is 8593467976
please help!!
Gary Rollins
On Sat, Apr 20, 2024, 11:46 AM DrahtBot ***@***.***> wrote:
> The following sections might be updated with supplementary metadata
> relevant to reviewers and maintainers.
> Code Coverage
>
> For detailed information about the code coverage, see the test coverage
> report <https://corecheck.dev/bitcoin/bitcoin/p
...
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2067721400)
Why am I receiving these emails?This person is using my email to hack my
Bitcoin account,please help, Gary Rollins my phone number is 8593467976
please help!!
Gary Rollins
On Sat, Apr 20, 2024, 11:46 AM DrahtBot ***@***.***> wrote:
> The following sections might be updated with supplementary metadata
> relevant to reviewers and maintainers.
> Code Coverage
>
> For detailed information about the code coverage, see the test coverage
> report <https://corecheck.dev/bitcoin/bitcoin/p
...
👋 fanquake's pull request is ready for review: "guix: remove bzip2 from deps"
(https://github.com/bitcoin/bitcoin/pull/29895)
(https://github.com/bitcoin/bitcoin/pull/29895)
💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345116)
done
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345116)
done
💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345292)
Not sure I understand your question, do you mean this line in particular or the whole hooking thing? This line was added the with the rest of hooking code in #18504 and I don't anything was changed about that until now, so I don't know where we might have forgotten to remove it.
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345292)
Not sure I understand your question, do you mean this line in particular or the whole hooking thing? This line was added the with the rest of hooking code in #18504 and I don't anything was changed about that until now, so I don't know where we might have forgotten to remove it.
💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345315)
Thanks, added.
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345315)
Thanks, added.
💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345459)
I think this is less readable so I will keep it as is but I did take @Sjors 's suggestion to change it to change `ptr` to `_`.
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345459)
I think this is less readable so I will keep it as is but I did take @Sjors 's suggestion to change it to change `ptr` to `_`.
💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345552)
I think this is less readable so I will keep it as is but I did take @Sjors 's suggestion to change it to change `ptr` to `_`.
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345552)
I think this is less readable so I will keep it as is but I did take @Sjors 's suggestion to change it to change `ptr` to `_`.
💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345672)
Like above, I think this is a bit less readable so I will keep it as is.
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345672)
Like above, I think this is a bit less readable so I will keep it as is.
💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345724)
Either way, someone might get confused, but I think it's better to keep it as is.
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573345724)
Either way, someone might get confused, but I think it's better to keep it as is.
💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573347809)
You're right, good catch! I was focussing too much on the behavior inside of libevent and didn't take into account that the result was cast to `std::string` afterwards, which lead to the behavior that a null character terminated the string. I have changed this to keep behavior consistent with the libevent version.
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1573347809)
You're right, good catch! I was focussing too much on the behavior inside of libevent and didn't take into account that the result was cast to `std::string` afterwards, which lead to the behavior that a null character terminated the string. I have changed this to keep behavior consistent with the libevent version.
💬 fjahr commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2067736339)
Addressed feedback by @paplorinc and @Sjors , thanks a lot!
I also reorganized the commits a bit because they were doing more than one thing and it was hard to address feedback cleanly. The tests are now split from the implementation, which makes it easier to cherry-pick the unit test commit and run it against master. And I split off the renaming into a scripted diff.
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2067736339)
Addressed feedback by @paplorinc and @Sjors , thanks a lot!
I also reorganized the commits a bit because they were doing more than one thing and it was hard to address feedback cleanly. The tests are now split from the implementation, which makes it easier to cherry-pick the unit test commit and run it against master. And I split off the renaming into a scripted diff.
🤔 sr-gi reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2013113076)
Second pass (up to the same height: 42859548ab5aebf40da6089b85065f7c204b992a)
Left some additional comments on the updated approach.
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2013113076)
Second pass (up to the same height: 42859548ab5aebf40da6089b85065f7c204b992a)
Left some additional comments on the updated approach.
💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573341141)
In ca9f03f2207854c849a14d7af2fcd91a5f675e14
Wouldn't it be easier (and simpler) to use a set here instead of a vector (as used to be the case in 9dc967195c4965973be0174ae6041be70a886c7a). It makes sense to use a `vec` in `GetChildrenFromSamePeer` given you are sorting based on a custom order, but here a set should be equivalent and less verbose (?)
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573341141)
In ca9f03f2207854c849a14d7af2fcd91a5f675e14
Wouldn't it be easier (and simpler) to use a set here instead of a vector (as used to be the case in 9dc967195c4965973be0174ae6041be70a886c7a). It makes sense to use a `vec` in `GetChildrenFromSamePeer` given you are sorting based on a custom order, but here a set should be equivalent and less verbose (?)
💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573354857)
In 42859548ab5aebf40da6089b85065f7c204b992a
To make sure the two collections are equal you also need to check the their sizes match. This applies to the four cases.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573354857)
In 42859548ab5aebf40da6089b85065f7c204b992a
To make sure the two collections are equal you also need to check the their sizes match. This applies to the four cases.
💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573331603)
In 5c8aa657642ef24e711a73c28278644f14117d73
This last check is still redundant
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573331603)
In 5c8aa657642ef24e711a73c28278644f14117d73
This last check is still redundant
💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573353122)
In 42859548ab5aebf40da6089b85065f7c204b992a
I don't think the order of recency is being tested
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573353122)
In 42859548ab5aebf40da6089b85065f7c204b992a
I don't think the order of recency is being tested
💬 sr-gi commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573344593)
In ca9f03f2207854c849a14d7af2fcd91a5f675e14
I think this is misleading (both the variable name and the comment).
The reason a vector is chosen is so you can sort based on `nTimeExpire`. Removing the duplicates comes later (for most of the function, the iters may not be unique).
For the sake of future readers it may be worth changing it
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1573344593)
In ca9f03f2207854c849a14d7af2fcd91a5f675e14
I think this is misleading (both the variable name and the comment).
The reason a vector is chosen is so you can sort based on `nTimeExpire`. Removing the duplicates comes later (for most of the function, the iters may not be unique).
For the sake of future readers it may be worth changing it
🤔 hebasto reviewed a pull request: "depends: build libnatpmp with CMake"
(https://github.com/bitcoin/bitcoin/pull/29708#pullrequestreview-2013138028)
Approach ACK 3c1ae3ee33d4d9dbea046d5ab8ee924a12982759.
I've verified compiler flags for different scenarios, including cross-compiling for Windows and providing `DEBUG=1`. CMake ones lack `-Wall -Wextra` flags, but that's not a blocker.
Also this PR limits the installed headers to ones that are actually used in our code, which is fine
(https://github.com/bitcoin/bitcoin/pull/29708#pullrequestreview-2013138028)
Approach ACK 3c1ae3ee33d4d9dbea046d5ab8ee924a12982759.
I've verified compiler flags for different scenarios, including cross-compiling for Windows and providing `DEBUG=1`. CMake ones lack `-Wall -Wextra` flags, but that's not a blocker.
Also this PR limits the installed headers to ones that are actually used in our code, which is fine
💬 luke-jr commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2067767242)
Many people here conflating consensus (as modified by BIPs) with spam filters/policy. The reason why it's reasonable to relax consensus rules is in a large part *because* spam is mitigated by policy. If policy is to be discarded, we will need to impose all these limits strictly in consensus - that has not been proposed, and as such, reasonable policies must be maintained until it is. You can't just throw out an important security check without replacing it by something else!
Taproot's purpose
...
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2067767242)
Many people here conflating consensus (as modified by BIPs) with spam filters/policy. The reason why it's reasonable to relax consensus rules is in a large part *because* spam is mitigated by policy. If policy is to be discarded, we will need to impose all these limits strictly in consensus - that has not been proposed, and as such, reasonable policies must be maintained until it is. You can't just throw out an important security check without replacing it by something else!
Taproot's purpose
...