💬 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
...
💬 nflatrea commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2067767293)
> > Don't we have plethora of well documented guides and specifications on the JSON-RPC api ?
>
> No. I'd highly doubt that there is a better source of the documentation and specification than Bitcoin Core itself. (If there is, then that is a bug in Bitcoin Core, which should be fixed)
>
> > https://developer.bitcoin.org/reference/rpc/
>
> This example has many issues:
>
> * It is not machine readable
>
> * It does not indicate the version of Bitcoin Core that is documente
...
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-2067767293)
> > Don't we have plethora of well documented guides and specifications on the JSON-RPC api ?
>
> No. I'd highly doubt that there is a better source of the documentation and specification than Bitcoin Core itself. (If there is, then that is a bug in Bitcoin Core, which should be fixed)
>
> > https://developer.bitcoin.org/reference/rpc/
>
> This example has many issues:
>
> * It is not machine readable
>
> * It does not indicate the version of Bitcoin Core that is documente
...
💬 hebasto commented on issue "Apple Clang 14.0 lacks support for `std::is_eq`":
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2067769916)
@furszy
> Faced this too. Updating to 14.0.3 fixes the problem.
Just to clarify, what macOS version did you update your clang on?
(https://github.com/bitcoin/bitcoin/issues/29918#issuecomment-2067769916)
@furszy
> Faced this too. Updating to 14.0.3 fixes the problem.
Just to clarify, what macOS version did you update your clang on?
⚠️ Christewart opened an issue: "`keypoolrefill` doesn't fill keypool to specified parameter"
(https://github.com/bitcoin/bitcoin/issues/29924)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
According to the help manual the parameter is `newsize` and `keypoolrefill` fills the keypool to `newsize`. This was the behavior up to at least v21 of bitcoind. In v24 of bitcoind these semantics have changed. Related to https://github.com/bitcoin-s/bitcoin-s/pull/5496
```
$ bitcoin-cli -regtest getwalletinfo
{
"walletname": "",
"walletversion": 169900,
"format": "sqlite",
...
(https://github.com/bitcoin/bitcoin/issues/29924)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
According to the help manual the parameter is `newsize` and `keypoolrefill` fills the keypool to `newsize`. This was the behavior up to at least v21 of bitcoind. In v24 of bitcoind these semantics have changed. Related to https://github.com/bitcoin-s/bitcoin-s/pull/5496
```
$ bitcoin-cli -regtest getwalletinfo
{
"walletname": "",
"walletversion": 169900,
"format": "sqlite",
...
💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2067781043)
Re https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2006662114 and https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2001088868
> For example, replacing all uses of ChainstateLoadOptions::reindex with BlockManager::m_reindex is a trivial code change and passes unit and functional tests
...
> I also really like stickies suggestion from https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2001088868 to delete the ChainstateLoadOptions::reindex variable
...
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2067781043)
Re https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2006662114 and https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2001088868
> For example, replacing all uses of ChainstateLoadOptions::reindex with BlockManager::m_reindex is a trivial code change and passes unit and functional tests
...
> I also really like stickies suggestion from https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2001088868 to delete the ChainstateLoadOptions::reindex variable
...