💬 maflcko commented on pull request "doc: fix broken relative md links":
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093058899)
Would it be easy to add mlc (or something like it) to the lint CI task? (Just as an idea for a follow-up, not for here)
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093058899)
Would it be easy to add mlc (or something like it) to the lint CI task? (Just as an idea for a follow-up, not for here)
💬 GregTonoski commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093065595)
> There are knock-on effects like due to this, p2sh redeemScripts couldn't be larger(since they had to be pushed on the stack), and bip37 bloom filters.
Are they "knock-on effects" or are the effects intentional?
>
> Unless there is an instance of 520 being replaced here that is erroneous, this is a strict grepping improvement.
Is the suggested name the best option? Why not another constant with another name, perhaps?
Is use case of OP_
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093065595)
> There are knock-on effects like due to this, p2sh redeemScripts couldn't be larger(since they had to be pushed on the stack), and bip37 bloom filters.
Are they "knock-on effects" or are the effects intentional?
>
> Unless there is an instance of 520 being replaced here that is erroneous, this is a strict grepping improvement.
Is the suggested name the best option? Why not another constant with another name, perhaps?
Is use case of OP_
💬 theuni commented on pull request "miniscript: make operator""_mst consteval":
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-2093071416)
utACK with or without @hebasto's additional changes.
(https://github.com/bitcoin/bitcoin/pull/28657#issuecomment-2093071416)
utACK with or without @hebasto's additional changes.
👍 ryanofsky approved a pull request: "refactor, test: Always initialize pointer"
(https://github.com/bitcoin/bitcoin/pull/30026#pullrequestreview-2038202995)
Code review ACK bd2de7ac591d7704b79304089ad1fb57e085da8b
(https://github.com/bitcoin/bitcoin/pull/30026#pullrequestreview-2038202995)
Code review ACK bd2de7ac591d7704b79304089ad1fb57e085da8b
💬 instagibbs commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093072872)
> Are they "knock-on effects" or are the effects intentional?
Doesn't matter for this PR, frankly. We need to be descriptive about the consensus bits in the code. I linked some historical background for your edification.
> Is the suggested name the best option? Why not another constant with another name, perhaps?
Probaby, because it's the constant that has been used for over a decade to describe a consensus-critical constant.
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093072872)
> Are they "knock-on effects" or are the effects intentional?
Doesn't matter for this PR, frankly. We need to be descriptive about the consensus bits in the code. I linked some historical background for your edification.
> Is the suggested name the best option? Why not another constant with another name, perhaps?
Probaby, because it's the constant that has been used for over a decade to describe a consensus-critical constant.
🚀 ryanofsky merged a pull request: "refactor, test: Always initialize pointer"
(https://github.com/bitcoin/bitcoin/pull/30026)
(https://github.com/bitcoin/bitcoin/pull/30026)
💬 sipa commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093078874)
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
> Is the suggested name the best option? Why not another constant with another name, perhaps?
The constant with this exact name and value already exists, it was introduced [11 years ago](https://github.com/bitcoin/bitcoin/pull/2188). There are just a few places in comments where the raw value 520 is used instead so far. This PR fixes that.
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093078874)
ACK ffc674595cb19b2fdc5705b355bdd3e7f724b860
> Is the suggested name the best option? Why not another constant with another name, perhaps?
The constant with this exact name and value already exists, it was introduced [11 years ago](https://github.com/bitcoin/bitcoin/pull/2188). There are just a few places in comments where the raw value 520 is used instead so far. This PR fixes that.
💬 ryanofsky commented on pull request "doc: fix broken relative md links":
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093081127)
Code review 8e394d1d3b6ead130515222f5b34d509fff200a8
I don't think it is good to change these to absolute links. When I open these files in emacs I can click the relative links and see the load the target files. If I change the links to be absolute, it tries to open non-existent paths on my root file system.
I imagine emacs is not the only tool which behaves this way. Is it necessary to make links absolute for broken link detection to work, or some other motivation to make them absolute?
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093081127)
Code review 8e394d1d3b6ead130515222f5b34d509fff200a8
I don't think it is good to change these to absolute links. When I open these files in emacs I can click the relative links and see the load the target files. If I change the links to be absolute, it tries to open non-existent paths on my root file system.
I imagine emacs is not the only tool which behaves this way. Is it necessary to make links absolute for broken link detection to work, or some other motivation to make them absolute?
💬 maflcko commented on pull request "doc: fix broken relative md links":
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093086045)
> Is it necessary to make links absolute for broken link detection to work, or some other motivation to make them absolute?
I brought it up, because absolute paths are already used, and it makes it more robust if a file or paragraph is moved. However, if mlc (or similar) is added in the future and checks against this, relative paths should be fine (and maybe preferred, if that helps with emacs)?
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093086045)
> Is it necessary to make links absolute for broken link detection to work, or some other motivation to make them absolute?
I brought it up, because absolute paths are already used, and it makes it more robust if a file or paragraph is moved. However, if mlc (or similar) is added in the future and checks against this, relative paths should be fine (and maybe preferred, if that helps with emacs)?
💬 willcl-ark commented on pull request "doc: fix broken relative md links":
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093095581)
The `mlc` tool seems fine with relative or absolute, so long as they resolve.
`nvim` seems to jump using relative or absolute links.
I did check to see if one or the other was more markdown-compliant, but it seems they purposefully avoided specifying. Seems like here we might prefer relative so we cater to emacs users, all other things being equal?
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093095581)
The `mlc` tool seems fine with relative or absolute, so long as they resolve.
`nvim` seems to jump using relative or absolute links.
I did check to see if one or the other was more markdown-compliant, but it seems they purposefully avoided specifying. Seems like here we might prefer relative so we cater to emacs users, all other things being equal?
💬 ryanofsky commented on pull request "doc: fix broken relative md links":
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093105387)
Oh, ok. I wasn't aware absolute links were already being used. I would slightly prefer links in the two files I wrote (libraries.md and multiprocess.md) to be relative so they work with emacs and so links within the files use a consistent style. But since I am the one responsible for a lot of these broken links, don't weigh my opinion too heavily :wink:
(https://github.com/bitcoin/bitcoin/pull/30025#issuecomment-2093105387)
Oh, ok. I wasn't aware absolute links were already being used. I would slightly prefer links in the two files I wrote (libraries.md and multiprocess.md) to be relative so they work with emacs and so links within the files use a consistent style. But since I am the one responsible for a lot of these broken links, don't weigh my opinion too heavily :wink:
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2093106374)
rebased due to conflict on master
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-2093106374)
rebased due to conflict on master
💬 maflcko commented on issue "ci: Support running from a worktree":
(https://github.com/bitcoin/bitcoin/issues/30028#issuecomment-2093142760)
I guess `macos` can just be skipped, as it will never run the base-install twice, so doesn't need caching.
(https://github.com/bitcoin/bitcoin/issues/30028#issuecomment-2093142760)
I guess `macos` can just be skipped, as it will never run the base-install twice, so doesn't need caching.
💬 jonatack commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093181592)
Thanks for the interesting links!
@GregTonoski it turns out that this patch is just a continuation of commit 192cc910ec7cade1d0dce7f3b111e7fc7720e607 doing the same thing, per review https://github.com/bitcoin/bitcoin/pull/2188#discussion_r2703506 requesting this, both back in 2013.
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2093181592)
Thanks for the interesting links!
@GregTonoski it turns out that this patch is just a continuation of commit 192cc910ec7cade1d0dce7f3b111e7fc7720e607 doing the same thing, per review https://github.com/bitcoin/bitcoin/pull/2188#discussion_r2703506 requesting this, both back in 2013.
💬 naiyoma commented on pull request "Test/rpc whitelistdefault test":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1589320996)
@tdb3 I've pushed an [update](https://github.com/bitcoin/bitcoin/pull/29858/commits/e02af85abaec67d92e976383777a4b0f2caae4e1). Adding the users to the existing list is a better approach. However, some tests are now failing because users are whitelisted in the run_test function, while the new tests are designed to test instances when a user is not whitelisted.
I thought clearing the bitcoin.conf file before writing the new test would solve this issue, but it doesn't. Below is a sample output
...
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1589320996)
@tdb3 I've pushed an [update](https://github.com/bitcoin/bitcoin/pull/29858/commits/e02af85abaec67d92e976383777a4b0f2caae4e1). Adding the users to the existing list is a better approach. However, some tests are now failing because users are whitelisted in the run_test function, while the new tests are designed to test instances when a user is not whitelisted.
I thought clearing the bitcoin.conf file before writing the new test would solve this issue, but it doesn't. Below is a sample output
...
💬 1440000bytes commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2093184542)
Concept ACK on adding another DNS seeder
> > Why does the seeder consider 'default port' for good nodes?
>
> DNS cannot provide port numbers, but a port must be known when connecting to a node. So we assume the default port, and because of that assumption, DNS seeders need to return nodes that are listening on the default port.
TXT records could work but that will require lot of other changes (out of scope)
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2093184542)
Concept ACK on adding another DNS seeder
> > Why does the seeder consider 'default port' for good nodes?
>
> DNS cannot provide port numbers, but a port must be known when connecting to a node. So we assume the default port, and because of that assumption, DNS seeders need to return nodes that are listening on the default port.
TXT records could work but that will require lot of other changes (out of scope)
🤔 furszy reviewed a pull request: "Refactor BnB tests"
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2034299860)
I'm not completely sure about 584e524eb57444d7192df1049cafde9ccc480406. The commit description says
> Originally these tests verified that at a SelectCoins level that a
> solution with fewer inputs gets preferred at high feerates, and a
> solution with more inputs gets preferred at low feerates. This outcome
> relies on the behavior of BnB, so we move these tests under the umbrella
> of BnB tests.
It is true that the outcome relies only on the BnB behavior currently but that might not
...
(https://github.com/bitcoin/bitcoin/pull/29532#pullrequestreview-2034299860)
I'm not completely sure about 584e524eb57444d7192df1049cafde9ccc480406. The commit description says
> Originally these tests verified that at a SelectCoins level that a
> solution with fewer inputs gets preferred at high feerates, and a
> solution with more inputs gets preferred at low feerates. This outcome
> relies on the behavior of BnB, so we move these tests under the umbrella
> of BnB tests.
It is true that the outcome relies only on the BnB behavior currently but that might not
...
💬 furszy commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1589243941)
In e286c6470b:
No need to clear `expected_result`. It's created within this function.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1589243941)
In e286c6470b:
No need to clear `expected_result`. It's created within this function.
💬 furszy commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1586712079)
In 3dff8f0490:
As far as I can see, the `nInput` arg is always 0 in this PR and also in #28985. Can we remove it?
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1586712079)
In 3dff8f0490:
As far as I can see, the `nInput` arg is always 0 in this PR and also in #28985. Can we remove it?
💬 furszy commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1586720038)
In https://github.com/bitcoin/bitcoin/commit/3dff8f0490b4b92c4adf229a828063dfda6d3a80:
I don't think we need all this includes. For instance, no wallet is used and `<wallet/wallet.h>` is included, no fee function is used and `wallet/fees.h` is included, `<random.h>` and `<random>` are included.
Maybe run IOWY to clean this up.
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1586720038)
In https://github.com/bitcoin/bitcoin/commit/3dff8f0490b4b92c4adf229a828063dfda6d3a80:
I don't think we need all this includes. For instance, no wallet is used and `<wallet/wallet.h>` is included, no fee function is used and `wallet/fees.h` is included, `<random.h>` and `<random>` are included.
Maybe run IOWY to clean this up.