💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720760697)
nit: `5` seems to be a random value, we might as well signal that with:
```suggestion
const std::string out_exp{"04678afdb0"};
const std::vector<char> in_s{HEX_PARSE_OUTPUT, HEX_PARSE_OUTPUT + out_exp.size() / 2};
```

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720759053)
nit: I'm not in love with the `ParseHex` -> `Vec(HexLiteral<uint8_t>` change, seems more noisy than needed.
What's the practical problem with keeping `ParseHex` in the tests?

💬 pinheadmz commented on something "":
(https://github.com/bitcoin/bitcoin/commit/1f6ab1215bbb1f8a5f1743c3c413b95ad08090df#r145479038)
this is test comment on a commit. it is intentionally spammy to test moderator bot. BUY MY $SCAMCOIN! email scam@scamcoin.xxx to get free money

💬 pinheadmz commented on something "":
(https://github.com/bitcoin/bitcoin/commit/1f6ab1215bbb1f8a5f1743c3c413b95ad08090df#r145479039)

💬 Christewart commented on pull request "addrman: change internal id counting to int64_t":
(https://github.com/bitcoin/bitcoin/pull/30568#issuecomment-2294849657)
Concept ACK

So presumably test cases are in in #22387 to directly test the vuln? I had a hard time deciphering what is meant to test rate limiting logic in #22387 and what test cases were for preventing a regression in the future.

💬 jonatack commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2294873445)
> it seems prudent to try with the value we "want" on testnet4, giving us the ability to learn if the compatibility concerns are an issue before we go to mainnet.

Concept ACK

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720781175)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720755562

> and many more, I think it would simplify a few trivial usages, hexadecimal encoding only makes sense for bigger values.

This would change which cscript overload is called, which could be good, but I think it would be better to do in a PR dedicated to improving cscript. That way this change can be focused on replacing runtime hex parsing with compile time parsing.

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720781544)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720757880

> Is `Vec` needed here because the existing `ToByteVector` has a long name? Or because of some funny visual studio exception? If none, maybe we could get rid of either.

CScript only supports << with std::vector, not std::array. It would be natural for it support std::array too, but I think that would be better be done in a followup improving CScript so this PR can be focused on replacing runtime hex parsing with compi
...

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720780412)
ddd

💬 ryanofsky commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720782497)
re: https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720759053

> nit: I'm not in love with the `ParseHex` -> `Vec(HexLiteral<uint8_t>` change, seems more noisy than needed. What's the practical problem with keeping `ParseHex` in the tests?

I feel like we could add a `HexLiteralU` or similar convenience function that is equivalent to `HexLiteral<uint8_t>`, because `HexLiteral<uint8_t>` does seem to be needed a lot of places after the `std::byte` change (107 instances currently, wh
...

💬 Sadgh65 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/ef4945f2218789d521da81fa9348a85eaef24b6f#commitcomment-145479989)
1.

💬 stratospher commented on pull request "addrman: change internal id counting to int64_t":
(https://github.com/bitcoin/bitcoin/pull/30568#issuecomment-2294881812)
> I had a hard time deciphering what is meant to test the new rate limiting logic in https://github.com/bitcoin/bitcoin/pull/22387 and what test cases were intended to prevent a regression for being introduced in the future that re-introduces this vulnerability.

Main thing is to make sure that `nId` values are distinct. #22387 limits the `nId` which the addrman can possibly construct([functional tests which test this behaviour](https://github.com/bitcoin/bitcoin/blob/ee367170cb2acf82b6ff8e0cc
...

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720801377)
(On phone, can't check)
Both of them produce a vector<unsigned char>, right?

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720801552)
I mean both Vec and ToByteVector would work here, right?

📝 haydenbanz opened a pull request: "modified version"
(https://github.com/bitcoin/bitcoin/pull/30668)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

✅ achow101 closed a pull request: "modified version"
(https://github.com/bitcoin/bitcoin/pull/30668)

📝 achow101 locked a pull request: "modified version"
(https://github.com/bitcoin/bitcoin/pull/30668)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 sipsorcery commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2294985333)
I'm doing some test builds on Windows and it was a bit of a treasure hunt to find the binaries. The relevant doc states "CMake will put the resulting object files, libraries, and executables into a dedicated build directory.". Would it be worth listing the directories?

The Autotools Linux based builds put the binaries into a single obvious location. If that's tricky with cmake then maybe the next best thing is to list the locations where the binaries can be found?

Shouldn't this PR also re
...

💬 paplorinc commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1720836846)
Thanks @ryanofsky

💬 fjahr commented on pull request "seeds: Pull additional nodes from my seeder and update fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/30008#issuecomment-2294996033)
re-ACK 41ad84a00c20f54b520aab7f6f975231da0ee2d0

Only changes were addressing above (minor) comments: https://github.com/bitcoin/bitcoin/compare/7f55140007186cda876ad0a5da812e391cddbcc4..41ad84a00c20f54b520aab7f6f975231da0ee2d0